Opened 5 months ago
Closed 4 months ago
#33588 closed defect (fixed)
warnings to ignore are not ignored when a doctest is tagged with # tol
Reported by:  slabbe  Owned by:  

Priority:  blocker  Milestone:  sage9.6 
Component:  numerical  Keywords:  
Cc:  fbissey  Merged in:  
Authors:  Sébastien Labbé  Reviewers:  John Palmieri 
Report Upstream:  N/A  Work issues:  
Branch:  2afb472 (Commits, GitHub, GitLab)  Commit:  2afb47293d0fb63f017d3e9acb6035166dfc92aa 
Dependencies:  Stopgaps: 
Description (last modified by )
The symptom
On Ubuntu 18.04, with SageMath version 9.6.beta6, Release Date: 20220327
, with the GLPK available on the system $ glpsol v
returning GLPSOL: GLPK LP/MIP Solver, v4.65
, we observe doctest failures of the following kind:
Failed example: lp.solve() # tol 1e8 # optional  sage.rings.number_field Expected: 1.3090169943749475 Got: Longstep dual simplex will be used 1.3090169943749472
Failures always consists of doctests tagged with some numerical tolerence # tol
. It seems the fix proposed in #29317 to ignore sentence Longstep dual simplex will be used
printed by GLPK does not work for doctests tagged with # tol
.
The complete list of failures consist of the following 4 files:
sage tp long src/sage/geometry/polyhedron/base.py src/sage/numerical/backends/glpk_backend.pyx src/sage/numerical/mip.pyx src/sage/tests/books/computationalmathematicswithsagemath/lp_doctest.py
gives
 sage t long src/sage/geometry/polyhedron/base.py # 1 doctest failed sage t long src/sage/numerical/backends/glpk_backend.pyx # 4 doctests failed sage t long src/sage/numerical/mip.pyx # 8 doctests failed sage t long src/sage/tests/books/computationalmathematicswithsagemath/lp_doctest.py # 2 doctests failed 
Isolation of the problem
The problem was understood in comment:7. The warnings are not ignored by check_output
method when tolerance tags are given. The following example illustrate the issue, the last line should be True:
sage: from sage.doctest.parsing import SageOutputChecker, SageDocTestParser sage: import doctest sage: optflag = doctest.NORMALIZE_WHITESPACEdoctest.ELLIPSIS sage: DTP = SageDocTestParser(('sage','magma','guava')) sage: OC = SageOutputChecker() sage: example = "sage: 1.3090169943749475 # tol 1e8\n1.3090169943749475" sage: ex = DTP.parse(example)[1] sage: ex.sage_source '1.3090169943749475 # tol 1e8\n' sage: ex.want '1.3090169943749475\n' sage: OC.check_output(ex.want, '1.3090169943749475', optflag) True sage: OC.check_output(ex.want, 'ANYTHING1.3090169943749475', optflag) False sage: OC.check_output(ex.want, 'Longstep dual simplex will be used\n1.3090169943749475', optflag) False
The problem with the current code
It can be checked inside of the check_output
method at line 925 and below that when a tolerance is given a result True or False is returned before any fixup is performed to remove eventual Longstep dual simplex will be used
to the output.
More precisely, at line 934, we return False because everything other than float numbers are not equal. This return False
is done too early. Because we want to do eventual fix ups before returning False.
The solution
The solution is not to return False at line 934 when the nonfloat part is not equal. In fact it is better to first check that all floats intervals intersect (currently done at line 942) and return False if not all intervals intersect.
Then, we can continue with checking the remaining nonfloats parts as it is done by default when no tolerance is given instead of returning False.
Further improvements
We create a independent method do_fixup
which does the removal of eventual warnings as a last resort solution before returning False.
To ease the review, we keep the list _repr_fixups
for now in the header of the module and leave its integration within do_fixup
method for the followup ticket #33680.
Change History (47)
comment:1 Changed 5 months ago by
 Description modified (diff)
comment:2 Changed 5 months ago by
comment:3 followup: ↓ 5 Changed 5 months ago by
comment:4 Changed 5 months ago by
 Description modified (diff)
comment:5 in reply to: ↑ 3 Changed 5 months ago by
Replying to jhpalmieri:
See also #29317, where we solved this problem once before. Is this with a system version of GLPK? I'm not sure why the fix at #29317 is not catching this, in fact.
Yes, it is the glpk from the system:
$ grep glpk config.log ## Checking whether SageMath should install SPKG glpk... ## configure:12313: checking glpk.h usability configure:12313: checking glpk.h presence configure:12313: checking for glpk.h configure:12352: g++ std=gnu++11 o conftest conftest.cpp lglpk lgmp lm >&5 configure:12369: result: lglpk configure:12374: result: yes. Use system's glpk configure:12395: will use system package and not install SPKG glpk configure:12434: result: using glpk from the system
This one:
$ glpsol v GLPSOL: GLPK LP/MIP Solver, v4.65
comment:6 followup: ↓ 7 Changed 5 months ago by
As I said, I wonder why the fix from #29317 is not catching this. From sage.doctest.parsing
:
# Version 4.65 of glpk prints the warning "Longstep dual simplex will # be used" frequently. When Sage uses a system installation of glpk # which has not been patched, we need to ignore that message. # See :trac:`29317`. glpk_simplex_warning_regex = re.compile(r'(Longstep dual simplex will be used)')
and then later on, that regular expression should be filtered out.
comment:7 in reply to: ↑ 6 Changed 5 months ago by
Replying to jhpalmieri:
As I said, I wonder why the fix from #29317 is not catching this.
I got it. It is because the # tol 1e8
is dealt in the check_output
method before the glpk regex get replaced.
The following behavior is fine:
sage: import doctest sage: optflag = doctest.NORMALIZE_WHITESPACEdoctest.ELLIPSIS sage: from sage.doctest.parsing import SageOutputChecker sage: OC = SageOutputChecker() sage: OC.check_output('1.3090169943749475','1.3090169943749475',optflag) True sage: OC.check_output('1.3090169943749475','ANYTHING1.3090169943749475',optflag) False sage: OC.check_output('1.3090169943749475','Longstep dual simplex will be used\n1.3090169943749475',optflag) True
The following is ok as well:
sage: from sage.doctest.parsing import SageOutputChecker, SageDocTestParser sage: import doctest sage: optflag = doctest.NORMALIZE_WHITESPACEdoctest.ELLIPSIS sage: DTP = SageDocTestParser(('sage','magma','guava')) sage: OC = SageOutputChecker() sage: example = "sage: 1.3090169943749475\n1.3090169943749475" sage: ex = DTP.parse(example)[1] sage: ex.want '1.3090169943749475\n' sage: OC.check_output(ex.want, '1.3090169943749475', optflag) True sage: OC.check_output(ex.want, 'ANYTHING1.3090169943749475', optflag) False sage: OC.check_output(ex.want, 'Longstep dual simplex will be used\n1.3090169943749475', optflag) True
The following illustrates the bug (the last line should be True):
sage: example = "sage: 1.3090169943749475 # tol 1e8\n1.3090169943749475" sage: ex = DTP.parse(example)[1] sage: ex.sage_source '1.3090169943749475 # tol 1e8\n' sage: ex.want '1.3090169943749475\n' sage: OC.check_output(ex.want, '1.3090169943749475', optflag) True sage: OC.check_output(ex.want, 'ANYTHING1.3090169943749475', optflag) False sage: OC.check_output(ex.want, 'Longstep dual simplex will be used\n1.3090169943749475', optflag) False
Observe that turning ex.want
back to a string fixes it because I think the return is made in the if isinstance(want, MarkedOutput):
block within the check_output
method:
sage: type(ex.want) <class 'sage.doctest.parsing.MarkedOutput'> sage: sage: OC.check_output(str(ex.want), 'Longstep dual simplex will be used\n1.3090169943749475', optflag) True
comment:8 Changed 5 months ago by
 Priority changed from major to critical
comment:9 Changed 4 months ago by
 Branch set to u/slabbe/33588
 Commit set to a67986cbc03474f19a3b699d3a44c1d5f36d6139
 Status changed from new to needs_review
comment:10 Changed 4 months ago by
The code as it is now works and fixes the issue in the description of this ticket. Doing so, I added a new method try_fixup
which helps understand what is happening. The way it is written follows how the code was written before. But here are few things that I dislike and that could be improved if reviewers agree:
 parameter
want
is not changed bytry_fixup
, onlygot
, so why shouldfixup
takewant
as input?  the list
_repr_fixups
is defined in the header of the module but it is used only in thetry_fixup
method. I think it would make sense to remove it from the header and move it down inside thetry_fixup
method. It would make thetry_fixup
method more usable. More precisely, when somethings goes wrong for instance while debugging #33588, it is practical to add fewprint
statements to see what is happening but it is quite painful to add a print statement inside of the lambda functions defined in the_repr_fixups
at the top of the module.  Since
_repr_fixups
is defined completely out of context in the top module it comes with 8 lines of documentation painfully explaining what it is. These lines would just disappear by defining them in context inside of thetry_fixup
method.
# Collection of fixups applied in the SageOutputChecker. Each element in this # this list a pair of functions applied to the actual test output ('g' for # "got") and the expected test output ('w' for "wanted"). The first function # should be a simple fast test on the expected and/or actual output to # determine if a fixup should be applied. The second function is the actual # fixup, which is applied if the test function passes. In most fixups only one # of the expected or received outputs are normalized, depending on the # application. _repr_fixups = [ (lambda g, w: "Longstep" in g, lambda g, w: (glpk_simplex_warning_regex.sub('', g), w)), (lambda g, w: "dylib" in g, lambda g, w: (ld_warning_regex.sub('', g), w)), (lambda g, w: "pie being ignored" in g, lambda g, w: (ld_pie_warning_regex.sub('', g), w)) ]
If reviewers agree with me, I will do one more commit improving that.
comment:11 Changed 4 months ago by
 Status changed from needs_review to needs_info
comment:12 Changed 4 months ago by
 Commit changed from a67986cbc03474f19a3b699d3a44c1d5f36d6139 to 0274a6afcaa42b00241ab492fcfa17229e2bdaa5
Branch pushed to git repo; I updated commit sha1. New commits:
0274a6a  33588: using ifelse is better at showing the logic structure of the cases

comment:13 Changed 4 months ago by
I just added a commit which is unrelated to the question asked in comment:10.
comment:14 Changed 4 months ago by
 Commit changed from 0274a6afcaa42b00241ab492fcfa17229e2bdaa5 to 1dcf54978ef9bfa6410f2e6d0db582c65cc73d8a
Branch pushed to git repo; I updated commit sha1. New commits:
1dcf549  33588: moving _repr_fixups from module header to method try_fixup

comment:15 Changed 4 months ago by
 Status changed from needs_info to needs_review
While having my head on this part of the code, I decide to answer my comment:10 by YES and I did the latest commit.
Needs review!
comment:16 Changed 4 months ago by
 Commit changed from 1dcf54978ef9bfa6410f2e6d0db582c65cc73d8a to cfac413ef73169d0b887fa8d3fcf72efecf221db
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cfac413  33588: moving _repr_fixups from module header to method try_fixup

comment:17 Changed 4 months ago by
 Commit changed from cfac413ef73169d0b887fa8d3fcf72efecf221db to 10dee4e8a1505d491f3963f5216c122c516e4de9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
10dee4e  33588: moving _repr_fixups from module header to method try_fixup

comment:18 Changed 4 months ago by
Did 2 small changes on the last commit. Now done. Needs review.
comment:19 Changed 4 months ago by
I don't have time to review this right now, but I agree with your decisions regarding comment:10.
comment:20 Changed 4 months ago by
 Cc fbissey added
comment:21 Changed 4 months ago by
 Commit changed from 10dee4e8a1505d491f3963f5216c122c516e4de9 to d7d40c770179177f1d3dfd857fbdc107aee4d29e
Branch pushed to git repo; I updated commit sha1. New commits:
d7d40c7  33588: renamed try_fixup > do_fixup because looks better like this

comment:22 Changed 4 months ago by
I renamed try_fixup
to do_fixup
because the following line looks better like this, the same verb is used:
did_fixup, want, got = self.do_fixup(want, got)
comment:23 Changed 4 months ago by
 Description modified (diff)
comment:24 Changed 4 months ago by
 Commit changed from d7d40c770179177f1d3dfd857fbdc107aee4d29e to 6f206182e8545081bfeb67958cd8472b04f5f5c9
Branch pushed to git repo; I updated commit sha1. New commits:
6f20618  33588:OUTPUT > OUTPUT:

comment:25 Changed 4 months ago by
I added a commit to fix one patchbot warning:
========== blocks ========== git checkout patchbot/ticket_merged inside file: b/src/sage/doctest/parsing.py @@ 923,41 +919,111 @@ class SageOutputChecker(doctest.OutputChecker): + OUTPUT wrong syntax for blocks (INPUT, OUTPUT, EXAMPLES, NOTE, etc) inserted on 1 nonempty lines blocks  0 seconds ========== end blocks ==========
comment:26 Changed 4 months ago by
Please do not remove _repr_fixups
and the comments before.
EDIT: Just make that an empty list
Somebody else may need that again at some point.
comment:27 Changed 4 months ago by
ah, I see, this is more complicated than what I thought. No time to look at that now.
comment:28 Changed 4 months ago by
It is not so complicated in the end, but it is true that it may look complicated.
Let me update the description of the ticket to better explain the problem and the solution.
comment:29 Changed 4 months ago by
 Commit changed from 6f206182e8545081bfeb67958cd8472b04f5f5c9 to 4a4cd4bf7e1a36970e6662b1372f58aea2968939
comment:30 Changed 4 months ago by
I removed the commit deleting the _repr_fixups
as it can be done in a later ticket which will simplify the review of the current ticket.
comment:33 Changed 4 months ago by
Needs review.
comment:34 Changed 4 months ago by
 Description modified (diff)
comment:35 Changed 4 months ago by
 Summary changed from 15 GLPK doctests failing because "Longstep dual simplex will be used" gets printed to "Longstep dual simplex will be used" sentence is not ignored when doctest is tagged with # tol
comment:36 Changed 4 months ago by
 Summary changed from "Longstep dual simplex will be used" sentence is not ignored when doctest is tagged with # tol to "Longstep dual simplex will be used" warning is not ignored when doctest is tagged with # tol
comment:37 Changed 4 months ago by
 Description modified (diff)
Touching the _repr_fixups
list is now in a followup ticket #33680.
comment:38 Changed 4 months ago by
 Summary changed from "Longstep dual simplex will be used" warning is not ignored when doctest is tagged with # tol to warnings to ignore are not ignored when a doctest is tagged with # tol
comment:39 Changed 4 months ago by
 Description modified (diff)
comment:40 Changed 4 months ago by
 Status changed from needs_review to needs_work
one patchbot currently says:
[sagemath_doc_htmlnone] Error building the documentation. [sagemath_doc_htmlnone] Traceback (most recent call last): ... [sagemath_doc_htmlnone] OSError: /home/debian/sage/local/var/lib/sage/ venvpython3.9/lib/python3.9/sitepackages/sage/doctest/parsing.py: docstring of sage.doctest.parsing.SageOutputChecker.check_output:118: WARNING: Block quote ends without a blank line; unexpected unindent. [sagemath_doc_htmlnone]
comment:41 Changed 4 months ago by
 Commit changed from 4a4cd4bf7e1a36970e6662b1372f58aea2968939 to 2afb47293d0fb63f017d3e9acb6035166dfc92aa
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
11f7a34  33588: adding do_fixup method in the SageOutputChecker

1d337e4  33588: continue to compare the nonfloats part of doctests when tolerance is given

79071f5  33588: using ifelse is better at showing the logic structure of the cases

2afb472  33588: rawstring for documentation of check_output

comment:42 Changed 4 months ago by
The documentation failure was due to a docstring defined with """..."""
as opposed to r"""..."""
. I added a commit to fix that.
Now rebased on 9.6.rc0.
Also squashed the commit changing name try_fixup
to do_fixup
to simplify the review.
comment:43 Changed 4 months ago by
 Status changed from needs_work to needs_review
comment:44 Changed 4 months ago by
 Reviewers set to John Palmieri
 Status changed from needs_review to positive_review
This looks good to me.
comment:45 Changed 4 months ago by
Thank you John!
comment:46 Changed 4 months ago by
 Priority changed from critical to blocker
comment:47 Changed 4 months ago by
 Branch changed from u/slabbe/33588 to 2afb47293d0fb63f017d3e9acb6035166dfc92aa
 Resolution set to fixed
 Status changed from positive_review to closed
but that is
9.6.beta6
+ #31962