Opened 11 years ago
Closed 10 years ago
#12493 closed defect (fixed)
tol and optional in doctests don't play well together
Reported by: | kcrisman | Owned by: | mvngu |
---|---|---|---|
Priority: | blocker | Milestone: | sage-5.0 |
Component: | doctest coverage | Keywords: | |
Cc: | mstreng, tmonteil, mjo | Merged in: | sage-5.0.beta14 |
Authors: | John Palmieri | Reviewers: | Marco Streng |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
See this sage-release thread and posts by Marco Streng. This bug is caused by #10923.
sage -t -only-optional=magma "devel/sage/sage/symbolic/integration/integral.py" was fine on beta2, but fails on beta3. This file has no magma doctests, but does have mathematica and maple doctests. The issue seems to be related to the doctesting framework, but I don't see any relevant tickets mentioned below. Could someone check whether this test works for them? Here's the traceback: sage -t -only-optional=magma "devel/sage/sage/symbolic/integration/integral.py" Traceback (most recent call last): File "/storage/marco/sage-5.0.beta3/local/bin/sage-doctest", line 924, in <module> test_file(argv[1], library_code = library_code) File "/storage/marco/sage-5.0.beta3/local/bin/sage-doctest", line 721, in test_file s = extract_doc(file, library_code=library_code) File "/storage/marco/sage-5.0.beta3/local/bin/sage-doctest", line 551, in extract_doc doc = doc_preparse(F[i:j+3]) File "/storage/marco/sage-5.0.beta3/local/bin/sage-doctest", line 370, in doc_preparse v = [i for i in range(len(t)) if only_optional_include(comment_modifiers[i])] IndexError: list index out of range [0.1 s]
This is verified on several systems.
Apply trac_12493.patch to the scripts repo.
Attachments (1)
Change History (21)
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
Removing the final empty line changes things, though it doesn't help.
sage: error.numerical_approx() # abs tol 10e-10 0 - """ 340 338
I can confirm that manually getting rid of this causes the tests to pass. A lot of trial and error as to which lines cause the problem has had no luck so far...
comment:3 Changed 11 years ago by
Okay, it's these lines.
sage: error.numerical_approx() # abs tol 10e-10 0
Somehow we must be using this abs tol thing incorrectly.
comment:4 Changed 11 years ago by
- Description modified (diff)
comment:5 Changed 11 years ago by
In case I can't track this down and someone from #10952 can help, here is the end of the comment modifiers.
' abs tol 10e-10', ' abs tol 10e-10', ' abs tol 10e-10', '']
comment:6 Changed 11 years ago by
I think it's something else in integral.py
(or, more likely, a combination of things). If I move the abs tol
test to another file, the tests all pass.
comment:7 Changed 11 years ago by
Replacing this comment with # optional -- internet requires
causes no problems. Same with # not tested
. But any other only optional (e.g., internet, maple) fails with the same problem.
Apparently the doctest framework has trouble with tol
and only-optional
. I'm making a note of this ticket at #10952.
comment:8 Changed 11 years ago by
Yup. Not guilty! Here's a minimal failing test case:
def dummy(): """ Test whether or not optional doctests and `abs tol` get along. sage: 1+1 # abs tol 1e-10 2 sage: 2+2 # optional - requires base 5 or higher 4 """ pass
comment:9 Changed 11 years ago by
LOL on the base 5 optional test! Though I wouldn't say it's minimal, strictly speaking - you actually explained yourself.
Hopefully someone at #10952 will fix this; I'm not interested in breaking the doctest script by fixing this in some stupid fashion.
comment:10 Changed 11 years ago by
- Summary changed from Fix doctest error in integration file to tol and optional in doctests don't play well together
comment:11 Changed 11 years ago by
- Description modified (diff)
- Status changed from new to needs_review
I think I've found the problem and fixed it, but please test it: try it on the Sage library, try it on files with doctests which are intentionally supposed to fail (modify dummy
above, for instance), both old-fashioned ones and ones with bad tolerance tests. It works for me, but it needs much more testing.
comment:12 Changed 11 years ago by
- Reviewers set to Marco Streng
- Status changed from needs_review to needs_work
I think the count is still wrong. I added a print statement to sage-doctest that lists the lines and the corresponding modifiers:
print "Printing lines and their modifiers" for i in range(len(t)): print "\n ========\n line:" print t[i] print "---\n comment:" print comment_modifiers[i] print "in total, there were %s lines with %s modifiers" % (len(t), len(comment_modifiers)) print "remaining lines:" print t[len(comment_modifiers):] print "remaining modifiers:" print comment_modifiers[len(t):]
Lines with tol give a mismatch, even with the patch.
As a consequence, the following file passes all tests with sage -t -only-optional=magma
, while it should fail!
def dummy(): """ Test whether or not optional doctests and `abs tol` get along. sage: 1+1 # rel tol 1e-10 2 sage: 2+2 # optional - requires magma and is wrong 1 """ pass
(13 lines, 15 modifiers, and the magma modifiers ended up on empty lines at the end, hence the pass with -only-optional=magma
)
The file fails the test with sage -t -optional
, as that runs all lines, regardless of modifiers. The file passes the test with sage -t
.
comment:13 Changed 11 years ago by
- Status changed from needs_work to needs_review
If you have a doctest marked "# optional - requires magma and is wrong", then it is supposed to only get tested if you run something like
sage -t --only-optional=requires,magma,and,is,wrong ...
Actually, there is a bug so it only gets tested if you run
sage -t --only-optional=requiresmagmaandiswrong ...
In any case, it's not surprising that your file passed doctests: the optional doctest wasn't run, because you didn't give the right command-line flags.
When I try your file with
sage -t --only-optional=requiresmagmaandiswrong ...
I get a doctest failure.
See #11615 for a fix to make --only-optional=requires,magma,and,is,wrong
work; the patch there also allows you to omit "requires" from this list.
comment:14 Changed 11 years ago by
Thanks for explaining why this particular example was wrong. Could you explain what is wrong with the rest of my comment too?
comment:15 Changed 11 years ago by
It looks like you're right, the new comment was being added too many times. Here's a new patch; try this instead.
comment:16 follow-up: ↓ 19 Changed 10 years ago by
All tests pass with the patch. But since this changes the doctesting framework, I needed to construct a doctest that is supposed to fail, and check whether it gives the correct failures. This made me stumble upon many problems with tol in doctesting.
Take this example:
""" EXAMPLES: The name blah is undefined:: sage: blah+blah blahblah Here's a doctest that is out of tolerance:: sage: 100+10000000000 # abs tol 0.1 10000000000 sage: 1+1 2 """
The output when doctesting is
File "/home/marco/sage-5.0.beta11/small.sage", line 6: sage: blah+blah Out of tolerance 10000000000.0 vs 10000000100.0
So the out of tolerance is mentioned for the wrong test. And the error of the test "blah+blah" is not even listed.
It gets worse if you remove the part that says
sage: 1+1 2
Then the python file produced for doctesting has a syntax error due to a blank line after the tolerance output.
File "/home/marco/.sage//tmp/small_8271.py", line 63 ... ''', res, 0.1, 'ab
Any test with tolerance followed by a blank line will produce a syntax error and end the doctesting of the whole file.
I guess I will have to work harder to find a set of doctests that avoids these issues, but still adequately tests whether trac_12493.patch is a good idea.
comment:17 Changed 10 years ago by
Yikes! Thanks for working hard to get something that actually works for this. Or you can just fix the tol
keyword :) I'll do my best to help review any changes if you do come up with a doctest patch.
comment:18 Changed 10 years ago by
- Status changed from needs_review to positive_review
comment:19 in reply to: ↑ 16 Changed 10 years ago by
comment:20 Changed 10 years ago by
- Merged in set to sage-5.0.beta14
- Resolution set to fixed
- Status changed from positive_review to closed
If we add print statement
we get the very interesting