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: sage-9.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:

Status badges

Description (last modified by slabbe)

The symptom

On Ubuntu 18.04, with SageMath version 9.6.beta6, Release Date: 2022-03-27, 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 1e-8   # optional - sage.rings.number_field
Expected:
    1.3090169943749475
Got:
    Long-step 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 Long-step 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/computational-mathematics-with-sagemath/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/computational-mathematics-with-sagemath/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_WHITESPACE|doctest.ELLIPSIS
sage: DTP = SageDocTestParser(('sage','magma','guava'))
sage: OC = SageOutputChecker()
sage: example = "sage: 1.3090169943749475 # tol 1e-8\n1.3090169943749475"
sage: ex = DTP.parse(example)[1]
sage: ex.sage_source
'1.3090169943749475 # tol 1e-8\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, 'Long-step 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 Long-step 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 non-float 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 non-floats 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 follow-up ticket #33680.

Change History (47)

comment:1 Changed 5 months ago by slabbe

  • Description modified (diff)

comment:2 Changed 5 months ago by gh-sheerluck

Doctesting 4 files.
sage -t --long --random-seed=19 geometry/polyhedron/base.py
    28 latte_int tests not run
    0 tests not run because we ran out of time
    [320 tests, 31.72 s]
sage -t --long --random-seed=19 numerical/backends/glpk_backend.pyx
    0 tests not run because we ran out of time
    [592 tests, 4.18 s]
sage -t --long --random-seed=19 numerical/mip.pyx
    8 cplex tests not run
    3 gurobi tests not run
    7 not tested tests not run
    0 tests not run because we ran out of time
    [713 tests, 1.60 s]
sage -t --long --random-seed=19 tests/books/computational-mathematics-with-sagemath/lp_doctest.py
    0 tests not run because we ran out of time
    [68 tests, 0.04 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 39.3 seconds
    cpu time: 20.1 seconds
    cumulative wall time: 37.5 seconds

but that is 9.6.beta6 + #31962

comment:3 follow-up: Changed 5 months ago by 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.

Last edited 5 months ago by jhpalmieri (previous) (diff)

comment:4 Changed 5 months ago by slabbe

  • Description modified (diff)

comment:5 in reply to: ↑ 3 Changed 5 months ago by slabbe

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 follow-up: Changed 5 months ago by jhpalmieri

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 "Long-step 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'(Long-step 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 slabbe

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 1e-8 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_WHITESPACE|doctest.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','Long-step 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_WHITESPACE|doctest.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, 'Long-step 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 1e-8\n1.3090169943749475"
sage: ex = DTP.parse(example)[1]
sage: ex.sage_source
'1.3090169943749475 # tol 1e-8\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, 'Long-step 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), 'Long-step dual simplex will be used\n1.3090169943749475', optflag)
True
Last edited 5 months ago by slabbe (previous) (diff)

comment:8 Changed 4 months ago by mkoeppe

  • Priority changed from major to critical

comment:9 Changed 4 months ago by slabbe

  • Authors set to Sébastien Labbé
  • Branch set to u/slabbe/33588
  • Commit set to a67986cbc03474f19a3b699d3a44c1d5f36d6139
  • Status changed from new to needs_review

I found a way to fix the issue.


New commits:

d17523133588: adding try_fixup method in the SageOutputChecker
a67986c33588: continue to compare the non-floats part of doctests when tolerance is given

comment:10 Changed 4 months ago by slabbe

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 by try_fixup, only got, so why should fixup take want as input?
  • the list _repr_fixups is defined in the header of the module but it is used only in the try_fixup method. I think it would make sense to remove it from the header and move it down inside the try_fixup method. It would make the try_fixup method more usable. More precisely, when somethings goes wrong for instance while debugging #33588, it is practical to add few print 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 the try_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: "Long-step" 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 slabbe

  • Status changed from needs_review to needs_info

comment:12 Changed 4 months ago by git

  • Commit changed from a67986cbc03474f19a3b699d3a44c1d5f36d6139 to 0274a6afcaa42b00241ab492fcfa17229e2bdaa5

Branch pushed to git repo; I updated commit sha1. New commits:

0274a6a33588: using if-else is better at showing the logic structure of the cases

comment:13 Changed 4 months ago by slabbe

I just added a commit which is unrelated to the question asked in comment:10.

comment:14 Changed 4 months ago by git

  • Commit changed from 0274a6afcaa42b00241ab492fcfa17229e2bdaa5 to 1dcf54978ef9bfa6410f2e6d0db582c65cc73d8a

Branch pushed to git repo; I updated commit sha1. New commits:

1dcf54933588: moving _repr_fixups from module header to method try_fixup

comment:15 Changed 4 months ago by slabbe

  • 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 git

  • Commit changed from 1dcf54978ef9bfa6410f2e6d0db582c65cc73d8a to cfac413ef73169d0b887fa8d3fcf72efecf221db

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cfac41333588: moving _repr_fixups from module header to method try_fixup

comment:17 Changed 4 months ago by git

  • Commit changed from cfac413ef73169d0b887fa8d3fcf72efecf221db to 10dee4e8a1505d491f3963f5216c122c516e4de9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

10dee4e33588: moving _repr_fixups from module header to method try_fixup

comment:18 Changed 4 months ago by slabbe

Did 2 small changes on the last commit. Now done. Needs review.

comment:19 Changed 4 months ago by jhpalmieri

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 fbissey

  • Cc fbissey added

comment:21 Changed 4 months ago by git

  • Commit changed from 10dee4e8a1505d491f3963f5216c122c516e4de9 to d7d40c770179177f1d3dfd857fbdc107aee4d29e

Branch pushed to git repo; I updated commit sha1. New commits:

d7d40c733588: renamed try_fixup -> do_fixup because looks better like this

comment:22 Changed 4 months ago by slabbe

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 slabbe

  • Description modified (diff)

comment:24 Changed 4 months ago by git

  • Commit changed from d7d40c770179177f1d3dfd857fbdc107aee4d29e to 6f206182e8545081bfeb67958cd8472b04f5f5c9

Branch pushed to git repo; I updated commit sha1. New commits:

6f2061833588:OUTPUT -> OUTPUT:

comment:25 Changed 4 months ago by slabbe

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 non-empty lines
blocks -- 0 seconds
========== end blocks ==========

comment:26 Changed 4 months ago by chapoton

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.

Last edited 4 months ago by chapoton (previous) (diff)

comment:27 Changed 4 months ago by chapoton

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 slabbe

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 git

  • Commit changed from 6f206182e8545081bfeb67958cd8472b04f5f5c9 to 4a4cd4bf7e1a36970e6662b1372f58aea2968939

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bcca55333588: renamed try_fixup -> do_fixup because looks better like this
4a4cd4b33588:OUTPUT -> OUTPUT:

comment:30 Changed 4 months ago by slabbe

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:31 Changed 4 months ago by slabbe

  • Description modified (diff)

Updated description.

comment:32 Changed 4 months ago by slabbe

  • Description modified (diff)

Further description improvements.

comment:33 Changed 4 months ago by slabbe

Needs review.

comment:34 Changed 4 months ago by slabbe

  • Description modified (diff)

comment:35 Changed 4 months ago by slabbe

  • Summary changed from 15 GLPK doctests failing because "Long-step dual simplex will be used" gets printed to "Long-step dual simplex will be used" sentence is not ignored when doctest is tagged with # tol

comment:36 Changed 4 months ago by slabbe

  • Summary changed from "Long-step dual simplex will be used" sentence is not ignored when doctest is tagged with # tol to "Long-step dual simplex will be used" warning is not ignored when doctest is tagged with # tol

comment:37 Changed 4 months ago by slabbe

  • Description modified (diff)

Touching the _repr_fixups list is now in a follow-up ticket #33680.

comment:38 Changed 4 months ago by slabbe

  • Summary changed from "Long-step 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 slabbe

  • Description modified (diff)

comment:40 Changed 4 months ago by slabbe

  • Status changed from needs_review to needs_work

one patchbot currently says:

[sagemath_doc_html-none] Error building the documentation.
[sagemath_doc_html-none] Traceback (most recent call last):
...
[sagemath_doc_html-none] OSError: /home/debian/sage/local/var/lib/sage/
venv-python3.9/lib/python3.9/site-packages/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_html-none]

comment:41 Changed 4 months ago by git

  • Commit changed from 4a4cd4bf7e1a36970e6662b1372f58aea2968939 to 2afb47293d0fb63f017d3e9acb6035166dfc92aa

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

11f7a3433588: adding do_fixup method in the SageOutputChecker
1d337e433588: continue to compare the non-floats part of doctests when tolerance is given
79071f533588: using if-else is better at showing the logic structure of the cases
2afb47233588: raw-string for documentation of check_output

comment:42 Changed 4 months ago by slabbe

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 slabbe

  • Status changed from needs_work to needs_review

comment:44 Changed 4 months ago by jhpalmieri

  • 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 slabbe

Thank you John!

comment:46 Changed 4 months ago by mkoeppe

  • Priority changed from critical to blocker

comment:47 Changed 4 months ago by vbraun

  • Branch changed from u/slabbe/33588 to 2afb47293d0fb63f017d3e9acb6035166dfc92aa
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.