Opened 8 years ago

Closed 8 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 jhpalmieri)

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)

trac_12493.patch (875 bytes) - added by jhpalmieri 8 years ago.
scripts repo

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by kcrisman

If we add print statement

            if contains_optional_doctests:
                # make a list of each optional line cut out by our tag choices
                print comment_modifiers
                print len(t), len(comment_modifiers)
                v = [i for i in range(len(t)) if only_optional_include(comment_modifiers[i])]

we get the very interesting

<snip lots of stuff>
341 339

comment:2 Changed 8 years ago by kcrisman

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 8 years ago by kcrisman

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 8 years ago by kcrisman

  • Description modified (diff)

comment:5 Changed 8 years ago by kcrisman

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 8 years ago by mjo

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 8 years ago by kcrisman

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 8 years ago by mjo

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 8 years ago by kcrisman

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 8 years ago by kcrisman

  • Summary changed from Fix doctest error in integration file to tol and optional in doctests don't play well together

comment:11 Changed 8 years ago by jhpalmieri

  • Authors set to John Palmieri
  • 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 8 years ago by mstreng

  • 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 8 years ago by jhpalmieri

  • 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 8 years ago by mstreng

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 8 years ago by jhpalmieri

It looks like you're right, the new comment was being added too many times. Here's a new patch; try this instead.

Changed 8 years ago by jhpalmieri

scripts repo

comment:16 follow-up: Changed 8 years ago by mstreng

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 8 years ago by kcrisman

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 8 years ago by mstreng

  • Status changed from needs_review to positive_review

comment:19 in reply to: ↑ 16 Changed 8 years ago by mstreng

Replying to mstreng:

So the out of tolerance is mentioned for the wrong test. And the error of the test "blah+blah" is not even listed.

This is a new ticket: #12815

comment:20 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta14
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.