Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#14466 closed defect (fixed)

issue with type in doctests

Reported by: Paul Zimmermann Owned by: David Roe
Priority: major Milestone: sage-5.10
Component: doctest framework Keywords:
Cc: Simon King Merged in: sage-5.10.beta1
Authors: Volker Braun, Nils Bruin Reviewers: Nils Bruin, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

as mentioned on https://groups.google.com/forum/?hl=fr&fromgroups=#!topic/sage-devel/wuwWfhWnbCg, there is an issue with type in doctests. When using type in an interactive session we get for example:

----------------------------------------------------------------------
| Sage Version 5.8, Release Date: 2013-03-15                         |
| Type "notebook()" for the browser-based notebook interface.        |
| Type "help()" for help.                                            |
----------------------------------------------------------------------
sage: type(ZZ)
sage.rings.integer_ring.IntegerRing_class

However, when we put that in a doctest, we get an error:

tarte% cat /tmp/a.py
def test():
    """
    EXAMPLE::

        sage: type(ZZ)
        sage.rings.integer_ring.IntegerRing_class
    """
    pass
tarte% /localdisk/tmp/sage-5.8/sage -t /tmp/a.py
sage -t  "/tmp/a.py"                                        
**********************************************************************
File "/tmp/a.py", line 5:
    sage: type(ZZ)
Expected:
    sage.rings.integer_ring.IntegerRing_class
Got:
    <type 'sage.rings.integer_ring.IntegerRing_class'>
**********************************************************************
1 items had failures:
   1 of   4 in __main__.example_0
***Test Failed*** 1 failures.
For whitespace errors, see the file /users/caramel/zimmerma/.sage//tmp/a_11238.py
         [2.1 s]
 
----------------------------------------------------------------------
The following tests failed:


        sage -t  "/tmp/a.py"
Total time for all tests: 2.2 seconds

A workaround is to put print type(...) in the doctest, but this prints the form <type 'sage.rings.integer_ring.IntegerRing_class'> which is not so nice...

apply trac_14466-nb.patch

Attachments (2)

trac_14466_fix_type_repr.patch (714 bytes) - added by Volker Braun 10 years ago.
Initial patch
trac_14466-nb.patch (963 bytes) - added by Nils Bruin 10 years ago.
include doctest

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by David Roe

There's a better workaround described in that thread: special case type in sage.misc.displayhook.format_obj.

comment:2 Changed 10 years ago by Paul Zimmermann

David, yes, however this workaround describes how to get <type ...> in the interactive session.

We want the opposite: get sage.rings.integer_ring.IntegerRing_class in the doctest, like the user gets in her/his interactive session.

Paul

comment:3 Changed 10 years ago by David Roe

I think that's a bad idea, since it means we have to change the result of a lot of doctests throughout Sage. Why not change both to be backward compatible?

comment:4 Changed 10 years ago by Paul Zimmermann

David, maybe I should explain the context: in our book in french about Sage, we give examples of Sage code that are both printed in the book, and also used to generate doctests (to ensure those examples will still run with future versions of Sage).

Thus we want that the result "Got" in doctests matches what we get in an interactive session. If this means we should add a special instruction at the beginning of our doctests, it is fine for us. No need to change all Sage doctests.

Paul

comment:5 Changed 10 years ago by David Roe

Yeah, that sounds feasible. You could have a switch in the first few lines of the file that changes it to interactive mode? Another possibility would be to special case this comparison in the check_output() function.

I'm going to sleep now and won't be online much in the next couple days.

comment:6 Changed 10 years ago by Simon King

Cc: Simon King added

comment:7 in reply to:  4 Changed 10 years ago by Nils Bruin

Replying to zimmerma:

David, maybe I should explain the context: in our book in french about Sage, we give examples of Sage code that are both printed in the book, and also used to generate doctests (to ensure those examples will still run with future versions of Sage).

Do you mean that book has already been printed? I'd say you can just update the book (immediately for the online version; in the next edition for a printed version, if that exists) to work correctly.

I think we've had a very small window where the new IPython has been printing different type representations.

It's pretty clear to me that we want to ensure that our doctests test the same output as produced by an interactive session (they are documentation as well as tests!).

The string <type '...'> is the straight str of a type. Changing the doctest framework to test against anything else in a robust way means that we'd have to change the doctest framework so that any return value gets passed through the same print hooks that IPython offers.

The alternative seems much easier: Change IPython to (mostly) just produce the straight str or repr.

Thus we want that the result "Got" in doctests matches what we get in an interactive session.

Which we can accomplish by ensuring that the interactive session does give the same result as it did a while ago. Are you particularly married to this new, modified interactive result we're getting due to the IPython upgrade?

EDIT: In fact, in the latest version 1.0.9, your book on page 47 has

sage: o = 12/35
sage: type(o)
<type 'sage.rings.rational.Rational'>

so I think you do want to go back to the old printing. We can easily do so by catching type objects in sage.misc.displayhook.format_obj. That unifies our printing again.

An additional reason to do so is that printing a type in the notebook still gives the normal <type '...'> result as well. I think that really shows that we don't want the special IPython behaviour in sage (doctest and notebook outvote IPython in my opinion).

Last edited 10 years ago by Nils Bruin (previous) (diff)

Changed 10 years ago by Volker Braun

Initial patch

comment:8 Changed 10 years ago by Volker Braun

Authors: Volker Braun
Status: newneeds_review

comment:9 Changed 10 years ago by Volker Braun

Anybody wants to review this trivial patch?

Changed 10 years ago by Nils Bruin

Attachment: trac_14466-nb.patch added

include doctest

comment:10 Changed 10 years ago by Nils Bruin

Turns out we can doctest this behaviour! See new patch. Also: if you compare print str('a') and print repr('a') you see that the latter is closer to what happens in the shell (not that it seems to matter for type objects)

Otherwise: of course a trivial patch (once you've dug out where to make the trivial change ...)

comment:11 Changed 10 years ago by Nils Bruin

patchbot apply trac_14466-nb.patch

so that we can see if this patch passes (Volker's does)

comment:12 Changed 10 years ago by Volker Braun

Authors: Volker BraunVolker Braun, Nils Bruin
Description: modified (diff)
Reviewers: Nils Bruin, Volker Braun
Status: needs_reviewpositive_review

Looks good to me.

comment:13 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.10.beta1
Resolution: fixed
Status: positive_reviewclosed

comment:14 Changed 10 years ago by Volker Braun

Nils managed to generate this commit message in Sage's hg log:

changeset:   18302:4c8dabfafa75
user:        Nils Bruin <nbruin@sfu.ca>
date:        Fri Apr 26 12:23:15 2013 -0700
summary:     Trac #14466: diff --git a/sage/misc/displayhook.py b/sage/misc/displayhook.py

comment:15 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:16 Changed 10 years ago by Jeroen Demeyer

There is also

changeset:   17832:42bc6caf1f2f
user:        Martin Raum <Martin.Raum@rwth-aachen.de>
date:        Tue Feb 19 10:32:37 2013 +0000
summary:     Trac #13531: diff --git a/sage/quadratic_forms/quadratic_form__automorphisms.py b/sage/quadratic_forms/quadratic_form__automorphisms.py

Merger script fixed such that this won't happen again, but rewriting history to fix the bad messages is probably a bad idea.

Last edited 10 years ago by Jeroen Demeyer (previous) (diff)
Note: See TracTickets for help on using tickets.