Opened 9 months ago
Closed 9 months ago
#33751 closed defect (fixed)
random doctest failure in src/sage/matrix/matrix_integer_dense_hnf.py
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: |
Description
Part of #32544:
sage -t --random-seed=47467259736041671371069989299524409608 src/sage/matrix/matrix_integer_dense_hnf.py ********************************************************************** File "src/sage/matrix/matrix_integer_dense_hnf.py", 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/forker.py", line 695, in _run self.compile_and_execute(example, compiler, test.globs) File "~/sage/src/sage/doctest/forker.py", 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/matrix_integer_dense_hnf.py", line 1285, in sanity_checks __do_check([random_matrix(ZZ, n, m, x=-2**32, y=2**32) File "~/sage/src/sage/matrix/matrix_integer_dense_hnf.py", line 1276, in __do_check if hnf(a)[0] != a.echelon_form(algorithm='pari'): File "~/sage/src/sage/matrix/matrix_integer_dense_hnf.py", line 1106, in hnf H, pivots = probable_hnf(A, include_zero_rows=include_zero_rows, File "~/sage/src/sage/matrix/matrix_integer_dense_hnf.py", line 926, in probable_hnf H = hnf_square(C, proof=proof) File "~/sage/src/sage/matrix/matrix_integer_dense_hnf.py", 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
comment:2 Changed 9 months ago by
But running something in an infinite loop until it works is also an awful idea, I guess.
comment:3 Changed 9 months ago by
Authors: | → Jonathan Kliem |
---|---|
Branch: | → public/33751 |
Commit: | → 7ac22ad241f631db06e2f725f23c67130f8f19b6 |
Status: | new → needs_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:
7ac22ad | catch value error of probable_hnf
|
comment:4 Changed 9 months ago by
Reviewers: | → Kevin Lui |
---|---|
Status: | needs_review → positive_review |
comment:6 Changed 9 months ago by
Priority: | minor → blocker |
---|
comment:7 Changed 9 months ago by
Branch: | public/33751 → 7ac22ad241f631db06e2f725f23c67130f8f19b6 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Note: See
TracTickets for help on using
tickets.
This is awful.
hnf
callsprobable_hnf
unconditionally.probable_hnf
clearly states that it might raise an exception and must be called again in this case. The documentation ofhnf
makes no such statements and I guess just assumes thatprobable_hnf
is always correct.