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: sage-8.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 chapoton)

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 chapoton

  • Branch set to u/chapoton/26628
  • Commit set to c6ce4b6a5748a8dcf483dbc6d24157132f5e0cfd
  • Status changed from new to needs_review

New commits:

5e262d0py3: fix doctests in c3_controlled
c6ce4b6py3: some fixes for sandpiles

comment:2 Changed 11 months ago by git

  • Commit changed from c6ce4b6a5748a8dcf483dbc6d24157132f5e0cfd to 7d3ecc74880f3c8970ab2eccc70c9cdaa4e10171

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

7d3ecc7py3: some fix for sandpiles

comment:3 Changed 11 months ago by git

  • Commit changed from 7d3ecc74880f3c8970ab2eccc70c9cdaa4e10171 to ba6fff38a08e5dc30071a4777fd68bba412b2c0d

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

ba6fff3trac 26628 fixing doctests

comment:4 Changed 11 months ago by chapoton

  • Cc tscrim added

bot is morally green, please review

comment:5 Changed 11 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:6 Changed 11 months ago by vbraun

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

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

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 chapoton

  • Status changed from new to needs_info

comment:10 Changed 10 months ago by chapoton

  • Status changed from needs_info to needs_work

comment:11 Changed 10 months ago by chapoton

  • Branch changed from ba6fff38a08e5dc30071a4777fd68bba412b2c0d to u/chapoton/26628
  • Commit set to ba6fff38a08e5dc30071a4777fd68bba412b2c0d

comment:12 Changed 10 months ago by git

  • Commit changed from ba6fff38a08e5dc30071a4777fd68bba412b2c0d to bfcaa32e04991f193af3f3d9a50a328646391ad6

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

0e0dddbMerge branch 'u/chapoton/26628' in 8.5.b6
bfcaa32trac 26628 repr of sandpiles

comment:13 Changed 10 months ago by chapoton

  • Status changed from needs_work to needs_review

Is this what you suggested, Volker ?

comment:14 Changed 10 months ago by chapoton

@vbraun:

or should I rather use from sage.repl.rich_output.pretty_print import pretty_print ?

comment:15 Changed 9 months ago by chapoton

  • Status changed from needs_review to needs_info

comment:16 Changed 9 months ago by chapoton

  • Milestone changed from sage-8.5 to sage-8.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 tscrim

  • Status changed from needs_review to positive_review

comment:18 Changed 9 months ago by vbraun

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

  • 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

NOTE: I am going to recycle this ticket right now.


New commits:

d976d7dsome care for sandpile tutorial
6be2430fix doctests

comment:20 Changed 9 months ago by chapoton

green bot, please review (ticket has been recycled to do something related)

comment:21 Changed 9 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:22 Changed 8 months ago by embray

  • 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 pretty-printed with its keys sorted. I haven't looked at SandpileConfig but it should probably do the same.

comment:23 Changed 8 months ago by embray

And does this relate in any way to #26722?

comment:24 follow-up: Changed 8 months ago by 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 ?

comment:25 in reply to: ↑ 24 Changed 8 months ago by embray

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 git

  • Commit changed from 6be243066df8a07b2542b40120ea38adad748409 to db899d8f6434055900457db3bb9367537f57ae7e

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

db899d8fix pretty printing of SandpileConfig and SandpileDivisor

comment:27 Changed 8 months ago by embray

  • Status changed from needs_info to needs_review

comment:28 Changed 8 months ago by chapoton

this will probably trigger the import of IPython at startup, no ?

comment:29 Changed 8 months ago by embray

I'm not sure what you mean.

comment:30 Changed 8 months ago by chapoton

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 pretty-printer in some ticket, and then the startup time got 10% longer (import of Ipython..)

comment:31 Changed 8 months ago by embray

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: 2019-01-03                 │
│ 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/site-packages/IPython/lib/pretty.pyc'>
sage: sys.modules['sage.sandpiles.sandpile']
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-4-8624d4f52316> in <module>()
----> 1 sys.modules['sage.sandpiles.sandpile']

KeyError: 'sage.sandpiles.sandpile'

comment:32 Changed 8 months ago by chapoton

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
Last edited 8 months ago by chapoton (previous) (diff)

comment:33 follow-up: Changed 8 months ago by embray

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 lazy-imported.

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 re-build that somehow triggers IPython from being imported.

Last edited 8 months ago by embray (previous) (diff)

comment:34 Changed 8 months ago by embray

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 embray

Replying to embray:

But then not thereafter. Something must change immediately after a re-build 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 chapoton

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Erik Bray
  • 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 embray

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 vbraun

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

Damnation...

comment:40 Changed 8 months ago by git

  • Commit changed from db899d8f6434055900457db3bb9367537f57ae7e to 8ee90c7e421565e6ecd98fd2b79f2589b01f7e41

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

a58cbbbMerge branch 'public/ticket/26628' in 8.6
8ee90c7sandpiles : fixing some doctests

comment:41 Changed 8 months ago by chapoton

  • Status changed from needs_work to needs_review

ok, this should fix it.

comment:42 Changed 8 months ago by chapoton

green bot. I have no way to test on OSX.

comment:43 Changed 8 months ago by chapoton

  • Cc vklein added

Vincent, would you please check the OsX behaviour ?

comment:44 Changed 8 months ago by vklein

Yes ! The two OSX errors from comment:38 are fixed.

comment:45 Changed 8 months ago by chapoton

  • Status changed from needs_review to positive_review

Merci beaucoup, Vincent. Je repasse en positif.

comment:46 Changed 8 months ago by embray

Weird. I'd have to take a closer look at how IPython's pretty-print module orders heterogeneous types when printing dictionaries. If it's doing something id-based then it's not going to reliable (at least not the default implementation ...)

comment:47 Changed 8 months ago by embray

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 more-or-less deterministic results on Python 3 (<3.7, where it relies on built-in dict ordering), but non-deterministic 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 vbraun

  • Branch changed from public/ticket/26628 to 8ee90c7e421565e6ecd98fd2b79f2589b01f7e41
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.