Opened 11 months ago
Closed 8 months ago
#26628 closed enhancement (fixed)
py3: some fixes for sandpiles tutorial
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  python3  Keywords:  
Cc:  tscrim, vklein  Merged in:  
Authors:  Frédéric Chapoton, Erik Bray  Reviewers:  Travis Scrimshaw, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  8ee90c7 (Commits)  Commit:  8ee90c7e421565e6ecd98fd2b79f2589b01f7e41 
Dependencies:  Stopgaps: 
Description (last modified by )
namely simple changes of notation for the sink vertex
This decreases strongly the number of failing doctests under py3 in this file.
Change History (48)
comment:1 Changed 11 months ago by
 Branch set to u/chapoton/26628
 Commit set to c6ce4b6a5748a8dcf483dbc6d24157132f5e0cfd
 Status changed from new to needs_review
comment:2 Changed 11 months ago by
 Commit changed from c6ce4b6a5748a8dcf483dbc6d24157132f5e0cfd to 7d3ecc74880f3c8970ab2eccc70c9cdaa4e10171
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7d3ecc7  py3: some fix for sandpiles

comment:3 Changed 11 months ago by
 Commit changed from 7d3ecc74880f3c8970ab2eccc70c9cdaa4e10171 to ba6fff38a08e5dc30071a4777fd68bba412b2c0d
Branch pushed to git repo; I updated commit sha1. New commits:
ba6fff3  trac 26628 fixing doctests

comment:5 Changed 11 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:6 Changed 11 months ago by
 Branch changed from u/chapoton/26628 to ba6fff38a08e5dc30071a4777fd68bba412b2c0d
 Resolution set to fixed
 Status changed from positive_review to closed
comment:7 Changed 10 months ago by
 Commit ba6fff38a08e5dc30071a4777fd68bba412b2c0d deleted
 Resolution fixed deleted
 Status changed from closed to new
I'm randomly getting this
********************************************************************** File "src/doc/en/thematic_tutorials/sandpile.rst", line 4678, in doc.en.thematic_tutorials.sandpile Failed example: D Expected: {1: 2, 'a': 0, 'b': 1} Got: {'a': 0, 'b': 1, 1: 2} ********************************************************************** 1 item had failures: 1 of 706 in doc.en.thematic_tutorials.sandpile [705 tests, 1 failure, 93.88 s] ********************************************************************** File "src/sage/sandpiles/sandpile.py", line 4769, in sage.sandpiles.sandpile.SandpileDivisor.values Failed example: D Expected: {1: 2, 'a': 0, 'b': 1} Got: {'a': 0, 'b': 1, 1: 2} ********************************************************************** 1 item had failures: 1 of 6 in sage.sandpiles.sandpile.SandpileDivisor.values [941 tests, 1 failure, 54.14 s]
comment:8 Changed 10 months ago by
Also, for consistency I'd recommend using src.sage.repl.display.pretty_print.SagePrettyPrinter? if you want to manually stringify an object
comment:9 Changed 10 months ago by
 Status changed from new to needs_info
comment:10 Changed 10 months ago by
 Status changed from needs_info to needs_work
comment:11 Changed 10 months ago by
 Branch changed from ba6fff38a08e5dc30071a4777fd68bba412b2c0d to u/chapoton/26628
 Commit set to ba6fff38a08e5dc30071a4777fd68bba412b2c0d
comment:12 Changed 10 months ago by
 Commit changed from ba6fff38a08e5dc30071a4777fd68bba412b2c0d to bfcaa32e04991f193af3f3d9a50a328646391ad6
comment:13 Changed 10 months ago by
 Status changed from needs_work to needs_review
Is this what you suggested, Volker ?
comment:14 Changed 10 months ago by
@vbraun:
or should I rather use from sage.repl.rich_output.pretty_print import pretty_print
?
comment:15 Changed 9 months ago by
 Status changed from needs_review to needs_info
comment:16 Changed 9 months ago by
 Milestone changed from sage8.5 to sage8.7
 Status changed from needs_info to needs_review
ok, the alternative with pretty_print does not make sense. Back to needs_review..
comment:17 Changed 9 months ago by
 Status changed from needs_review to positive_review
comment:18 Changed 9 months ago by
 Status changed from positive_review to needs_work
I'm still getting this on the OSX buildbot:
********************************************************************** File "src/doc/en/thematic_tutorials/sandpile.rst", line 4678, in doc.en.thematic_tutorials.sandpile Failed example: D Expected: {1: 2, 'a': 0, 'b': 1} Got: {'a': 0, 'b': 1, 1: 2} ********************************************************************** 1 item had failures: 1 of 706 in doc.en.thematic_tutorials.sandpile [705 tests, 1 failure, 86.65 s] ********************************************************************** File "src/sage/sandpiles/sandpile.py", line 4771, in sage.sandpiles.sandpile.SandpileDivisor.values Failed example: D Expected: {1: 2, 'a': 0, 'b': 1} Got: {'a': 0, 'b': 1, 1: 2} ********************************************************************** 1 item had failures: 1 of 6 in sage.sandpiles.sandpile.SandpileDivisor.values [941 tests, 1 failure, 43.18 s]  sage t long src/doc/en/thematic_tutorials/sandpile.rst # 1 doctest failed sage t long src/sage/sandpiles/sandpile.py # 1 doctest failed 
comment:19 Changed 9 months ago by
 Branch changed from u/chapoton/26628 to public/ticket/26628
 Commit changed from bfcaa32e04991f193af3f3d9a50a328646391ad6 to 6be243066df8a07b2542b40120ea38adad748409
 Description modified (diff)
 Status changed from needs_work to needs_review
 Summary changed from py3: some partial fix for sandpiles to py3: some fixes for sandpiles tutorial
comment:20 Changed 9 months ago by
green bot, please review (ticket has been recycled to do something related)
comment:21 Changed 9 months ago by
 Status changed from needs_review to positive_review
comment:22 Changed 8 months ago by
 Status changed from positive_review to needs_info
Something about this still smells off. For example:
@@ 3595,14 +3595,14 @@ boolean EXAMPLES::  sage: S = Sandpile({'a':[1,'b'], 'b':[1,'a'], 1:['a']},'a')  sage: c = SandpileConfig(S, {'b':1, 1:2}) + sage: S = Sandpile({'a':['c','b'], 'b':['c','a'], 'c':['a']},'a') + sage: c = SandpileConfig(S, {'b':1, 'c':2}) sage: c  {1: 2, 'b': 1} + {'c': 2, 'b': 1}
I'm not sure that last change is always going to be correct. If c
were a dict it would be prettyprinted with its keys sorted. I haven't looked at SandpileConfig
but it should probably do the same.
comment:23 Changed 8 months ago by
And does this relate in any way to #26722?
comment:24 followup: ↓ 25 Changed 8 months ago by
This is a preliminary cleanup work, certainly not the last word. Your concern should go to #26722. Sandpiles is a big mess, and I hope to move forward by small steps.
Can I set back to positive ?
comment:25 in reply to: ↑ 24 Changed 8 months ago by
Replying to chapoton:
This is a preliminary cleanup work, certainly not the last word. Your concern should go to #26722. Sandpiles is a big mess, and I hope to move forward by small steps.
Can I set back to positive ?
Unfortunately not: The problem is that since this is being repr'd like a dict, the output order is completely arbitrary, and this ticket is just changing it to a different arbitrary order that is not guaranteed to be the same between Python versions or even platforms.
If you give me a sec, I think I have a solution...
comment:26 Changed 8 months ago by
 Commit changed from 6be243066df8a07b2542b40120ea38adad748409 to db899d8f6434055900457db3bb9367537f57ae7e
Branch pushed to git repo; I updated commit sha1. New commits:
db899d8  fix pretty printing of SandpileConfig and SandpileDivisor

comment:27 Changed 8 months ago by
 Status changed from needs_info to needs_review
comment:28 Changed 8 months ago by
this will probably trigger the import of IPython at startup, no ?
comment:29 Changed 8 months ago by
I'm not sure what you mean.
comment:30 Changed 8 months ago by
I mean that the line
from IPython.lib import pretty
maybe will have a negative impact on sage startup time (if it imports all of IPython)
I am asking because I tried to import sage prettyprinter in some ticket, and then the startup time got 10% longer (import of Ipython..)
comment:31 Changed 8 months ago by
That's odd that you found that to be the case, because the Sage REPL is IPython:
$ ./sage ┌────────────────────────────────────────────────────────────────────┐ │ SageMath version 8.6.rc0, Release Date: 20190103 │ │ Using Python 2.7.15. Type "help()" for help. │ └────────────────────────────────────────────────────────────────────┘ ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ ┃ Warning: this is a prerelease version, and it may be unstable. ┃ ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ sage: import sys sage: sys.modules['IPython.lib.pretty'] <module 'IPython.lib.pretty' from '/home/embray/src/sagemath/sage/local/lib/python2.7/sitepackages/IPython/lib/pretty.pyc'> sage: sys.modules['sage.sandpiles.sandpile']  KeyError Traceback (most recent call last) <ipythoninput48624d4f52316> in <module>() > 1 sys.modules['sage.sandpiles.sandpile'] KeyError: 'sage.sandpiles.sandpile'
comment:32 Changed 8 months ago by
Yes, strange indeed, but see bottom report here:
https://patchbot.sagemath.org/ticket/26967/
and in particular the startup plugins..
EDIT: Apparently triggered by
+from sage.repl.rich_output.backend_base import BackendBase +from sage.repl.display.pretty_print import SagePrettyPrinter
comment:33 followup: ↓ 35 Changed 8 months ago by
Well, sage c
is different because it doesn't load the REPL, but it does import sage.all
. However, I still usually get:
$ ./sage c 'import sys; print("IPython" in sys.modules)' False
This might be in part because sage.sandpiles
is lazyimported.
For some odd reason though, immediately after a ./sage b
I get:
$ ./sage c 'import sys; print("IPython" in sys.modules)' True
But then not thereafter. Something must change immediately after a rebuild that somehow triggers IPython from being imported.
comment:34 Changed 8 months ago by
As my note says, it would be good to rework sage.repl.display.pretty_print
so that this kind of thing can be avoided in the first place since it's pretty ugly anyways.
comment:35 in reply to: ↑ 33 Changed 8 months ago by
Replying to embray:
But then not thereafter. Something must change immediately after a rebuild that somehow triggers IPython from being imported.
I see: That's in order to rebuild the star imports cache.
comment:36 Changed 8 months ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, then.
comment:37 Changed 8 months ago by
Thanks. It's still a good point you raise though: Merely importing those modules shouldn't cause IPython
to be imported if it can be avoided (e.g. it's pointless to do so if we're not in IPython).
I'll open a ticket for that.
comment:38 Changed 8 months ago by
 Status changed from positive_review to needs_work
On OSX:
********************************************************************** File "src/sage/sandpiles/sandpile.py", line 3486, in sage.sandpiles.sandpile.SandpileConfig.values Failed example: c Expected: {1: 2, 'b': 1} Got: {'b': 1, 1: 2} ********************************************************************** File "src/sage/sandpiles/sandpile.py", line 4765, in sage.sandpiles.sandpile.SandpileDivisor.values Failed example: D Expected: {1: 2, 'a': 0, 'b': 1} Got: {'a': 0, 'b': 1, 1: 2} **********************************************************************
comment:39 Changed 8 months ago by
Damnation...
comment:40 Changed 8 months ago by
 Commit changed from db899d8f6434055900457db3bb9367537f57ae7e to 8ee90c7e421565e6ecd98fd2b79f2589b01f7e41
comment:41 Changed 8 months ago by
 Status changed from needs_work to needs_review
ok, this should fix it.
comment:42 Changed 8 months ago by
green bot. I have no way to test on OSX.
comment:43 Changed 8 months ago by
 Cc vklein added
Vincent, would you please check the OsX behaviour ?
comment:44 Changed 8 months ago by
Yes ! The two OSX errors from comment:38 are fixed.
comment:45 Changed 8 months ago by
 Status changed from needs_review to positive_review
Merci beaucoup, Vincent. Je repasse en positif.
comment:46 Changed 8 months ago by
Weird. I'd have to take a closer look at how IPython's prettyprint module orders heterogeneous types when printing dictionaries. If it's doing something idbased then it's not going to reliable (at least not the default implementation ...)
comment:47 Changed 8 months ago by
Ugh, so it's using this:
def _sorted_for_pprint(items): """ Sort the given items for pretty printing. Since some predictable sorting is better than no sorting at all, we sort on the string representation if normal sorting fails. """ items = list(items) try: return sorted(items) except Exception: try: return sorted(items, key=str) except Exception: return items
Ironically this will give moreorless deterministic results on Python 3 (<3.7, where it relies on builtin dict ordering), but nondeterministic results on Python 2 where sorted(items)
will not raise an exception, and instead rely on Python 2's crappy fallback ordering which is not deterministic.
This might explain some other tests that expect dicts in their output, but where we've struggled to get consistent results across platforms. I'll look into fixing this.
comment:48 Changed 8 months ago by
 Branch changed from public/ticket/26628 to 8ee90c7e421565e6ecd98fd2b79f2589b01f7e41
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: fix doctests in c3_controlled
py3: some fixes for sandpiles