Opened 9 months ago

Closed 9 months ago

#33751 closed defect (fixed)

random doctest failure in src/sage/matrix/

Reported by: Lorenz Panny Owned by:
Priority: blocker Milestone: sage-9.6
Component: linear algebra Keywords:
Cc: Merged in:
Authors: Jonathan Kliem Reviewers: Kevin Lui
Report Upstream: N/A Work issues:
Branch: 7ac22ad (Commits, GitHub, GitLab) Commit: 7ac22ad241f631db06e2f725f23c67130f8f19b6
Dependencies: Stopgaps:

Status badges


Part of #32544:

sage -t --random-seed=47467259736041671371069989299524409608 src/sage/matrix/
File "src/sage/matrix/", line 1240, in sage.matrix.matrix_integer_dense_hnf.?
Failed example:
    matrix_integer_dense_hnf.sanity_checks(times=5, check_using_magma=False)
Exception raised:
    Traceback (most recent call last):
      File "~/sage/src/sage/doctest/", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "~/sage/src/sage/doctest/", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matrix.matrix_integer_dense_hnf.?[1]>", line 1, in <module>
        matrix_integer_dense_hnf.sanity_checks(times=Integer(5), check_using_magma=False)
      File "~/sage/src/sage/matrix/", line 1285, in sanity_checks
        __do_check([random_matrix(ZZ, n, m, x=-2**32, y=2**32)
      File "~/sage/src/sage/matrix/", line 1276, in __do_check
        if hnf(a)[0] != a.echelon_form(algorithm='pari'):
      File "~/sage/src/sage/matrix/", line 1106, in hnf
        H, pivots = probable_hnf(A, include_zero_rows=include_zero_rows,
      File "~/sage/src/sage/matrix/", line 926, in probable_hnf
        H = hnf_square(C, proof=proof)
      File "~/sage/src/sage/matrix/", line 568, in hnf_square
        raise ValueError("A must be square.")
    ValueError: A must be square.

(From a patchbot run in #33619.)

Change History (7)

comment:1 Changed 9 months ago by gh-kliem

This is awful.

hnf calls probable_hnf unconditionally. probable_hnf clearly states that it might raise an exception and must be called again in this case. The documentation of hnf makes no such statements and I guess just assumes that probable_hnf is always correct.

comment:2 Changed 9 months ago by gh-kliem

But running something in an infinite loop until it works is also an awful idea, I guess.

comment:3 Changed 9 months ago by gh-kliem

Authors: Jonathan Kliem
Branch: public/33751
Commit: 7ac22ad241f631db06e2f725f23c67130f8f19b6
Status: newneeds_review

I guess in the case of proof=True we just add another test to the loop. I still do not like the idea of running something in infinite loop (especially as I don't even know what it is doing).

But probable_hnf explicitly states that this can happen and that in this case it should be called again, so hnf with proof=True should account for this.

New commits:

7ac22adcatch value error of probable_hnf

comment:4 Changed 9 months ago by Kevin Lui

Reviewers: Kevin Lui
Status: needs_reviewpositive_review

comment:5 Changed 9 months ago by gh-kliem

Thank you.

comment:6 Changed 9 months ago by Matthias Köppe

Priority: minorblocker

comment:7 Changed 9 months ago by Volker Braun

Branch: public/337517ac22ad241f631db06e2f725f23c67130f8f19b6
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.