#26097 closed enhancement (fixed)
Py3: Some fixes in knot.link.py
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  python3  Keywords:  
Cc:  Erik Bray  Merged in:  
Authors:  Vincent Klein  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  b3f8dd2 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
In link.py
:
 regions(): use of a sorted list instead of a set for available_segments to have the same result in py2 and py3.
 _directions_of_edges(): Modify doctest to display results in the same order in py2 and py3.
Note to reviewer : As i am not an expert in knots theory please ensure that the new results of region()
are correct.
Also note that
if a in available_segments: available_segments.remove(a)
might be slower than before. Tell me if the performance impact is negligible or if i should use another algorithm.
Change History (24)
comment:1 Changed 4 years ago by
Summary:  Py3: Some fix in knot.link.py → Py3: Some fixes in knot.link.py 

comment:2 Changed 4 years ago by
Branch:  → u/vklein/py3__some_fixes_in_knot_link_py 

comment:3 Changed 4 years ago by
Commit:  → 5a7aed4361ee41cc8df0fbf4242ffd42dcf3bd8b 

Description:  modified (diff) 
comment:4 Changed 4 years ago by
Commit:  5a7aed4361ee41cc8df0fbf4242ffd42dcf3bd8b → 2d1ec036be5eed09981bfb30ff0da56c6f4b237e 

Branch pushed to git repo; I updated commit sha1. New commits:
2d1ec03  Trac #26097: Fix some doctests in ``knot.py``

comment:5 Changed 4 years ago by
Description:  modified (diff) 

comment:6 Changed 4 years ago by
Status:  new → needs_review 

comment:7 followup: 8 Changed 4 years ago by
beware of #25768, closed but not yet in the latest beta..
comment:8 Changed 4 years ago by
comment:9 followup: 11 Changed 4 years ago by
Cc:  Erik Bray added 

I think that the problem with doctests having a dictionary as result should rather be fixed globally in the doctest framework.
Maybe Erik has ideas about that ? or already a fix in general ??
comment:10 Changed 4 years ago by
I get the idea but i see one drawback: If the doctest framework manage to set a standard output order it might be confusing for the user because he might obtain another result order when copy/pasting the doctest in the sage interpreter.
comment:11 Changed 4 years ago by
Replying to chapoton:
I think that the problem with doctests having a dictionary as result should rather be fixed globally in the doctest framework.
Maybe Erik has ideas about that ? or already a fix in general ??
IIRC just a plain dict is generally not a problem because its repr is already pprinted in ipython. It's only a problem if the keys are heterogeneous.
comment:12 followup: 13 Changed 4 years ago by
FWIW on my python 3 branch I get:
$ ./sage t src/sage/knots/ too many failed tests, not using stored timings Running doctests with ID 20180828150702267ca96a. Git branch: python3 Using optional=meataxe,mpir,python2,sage Doctesting 4 files. sage t src/sage/knots/knot.py [42 tests, 8.48 s] sage t src/sage/knots/__init__.py [0 tests, 0.00 s] sage t src/sage/knots/all.py [0 tests, 0.00 s] sage t src/sage/knots/link.py ********************************************************************** File "src/sage/knots/link.py", line 1995, in sage.knots.link.Link.regions Failed example: L.regions() Expected: [[1, 7, 4], [2, 5, 7], [3, 8, 5], [4, 8], [6, 1, 3], [2, 6]] Got: [[1, 7, 4], [2, 5, 7], [3, 8, 5], [4, 8], [6, 1, 3], [6, 2]] ********************************************************************** File "src/sage/knots/link.py", line 2002, in sage.knots.link.Link.regions Failed example: L.regions() Expected: [[1, 3, 5], [2, 1], [4, 3], [6, 5], [2, 6, 4]] Got: [[1, 3, 5], [2, 1], [4, 3], [6, 5], [6, 4, 2]] ********************************************************************** File "src/sage/knots/link.py", line 2005, in sage.knots.link.Link.regions Failed example: L.regions() Expected: [[1, 5], [2, 8, 4, 5], [3, 8], [6, 9, 2], [7, 3, 9], [10, 4, 7], [10, 6, 1]] Got: [[1, 5], [2, 8, 4, 5], [3, 8], [6, 9, 2], [7, 3, 9], [10, 4, 7], [1, 10, 6]] ********************************************************************** File "src/sage/knots/link.py", line 2008, in sage.knots.link.Link.regions Failed example: L.regions() Expected: [[3], [4], [6], [8], [1, 2, 5, 7], [2, 3, 1, 8, 7, 6, 5, 4]] Got: [[3], [4], [6], [8], [1, 2, 5, 7], [1, 8, 7, 6, 5, 4, 2, 3]] ********************************************************************** 1 item had failures: 4 of 17 in sage.knots.link.Link.regions [413 tests, 4 failures, 8.36 s]  sage t src/sage/knots/link.py # 4 doctests failed  Total time for all tests: 17.1 seconds cpu time: 9.1 seconds cumulative wall time: 16.8 seconds
So I think some of these other tests are fixed elsewhere.
comment:13 Changed 4 years ago by
Replying to embray:
FWIW on my python 3 branch I get:
So I think some of these other tests are fixed elsewhere.
It might be something on your branch because i still get _directions_of_edges
doctests error with sage version 8.4.beta2.
$ sage t long src/sage/knots/link.py ... 2 items had failures: 3 of 7 in sage.knots.link.Link._directions_of_edges 4 of 17 in sage.knots.link.Link.regions [414 tests, 7 failures, 5.01 s]
comment:14 followup: 15 Changed 4 years ago by
True, that one might be just luck, because I'm not sure why it would appear "fixed" otherwise. However, normally a plain dict as output should not be a problemin this case it might fail only because it's a tuple of dicts. I think it would work if the tests were changed to something (which I find more instructive anyways) like:
sage: tails, heads = L._directions_of_edges() sage: tails ... sage: heads ...
in other words, printing each dict individually should work.
comment:15 Changed 4 years ago by
Replying to embray:
in other words, printing each dict individually should work.
Sadly, if i am not missing something, it doesn't :
Py2
sage: L = Link([[1, 3, 2, 4], [2, 3, 1, 4]]) sage: tails, heads = L._directions_of_edges() sage: tails {1: [2, 3, 1, 4], 2: [1, 3, 2, 4], 3: [1, 3, 2, 4], 4: [2, 3, 1, 4]} sage: heads {1: [1, 3, 2, 4], 2: [2, 3, 1, 4], 3: [2, 3, 1, 4], 4: [1, 3, 2, 4]}
Py3
... sage: tails {2: [1, 3, 2, 4], 1: [2, 3, 1, 4], 3: [1, 3, 2, 4], 4: [2, 3, 1, 4]} sage: heads {2: [2, 3, 1, 4], 1: [1, 3, 2, 4], 3: [2, 3, 1, 4], 4: [1, 3, 2, 4]}
Anyway i think your idea of printing each dict separately is better than having a for
loop.
comment:16 Changed 4 years ago by
Commit:  2d1ec036be5eed09981bfb30ff0da56c6f4b237e → 33c95fbf5ea95077e8d7f1d7f300556c7df034ba 

comment:18 Changed 4 years ago by
Commit:  33c95fbf5ea95077e8d7f1d7f300556c7df034ba → b3f8dd255b2adccb8c9a2b2bb6f148c6920bade5 

Branch pushed to git repo; I updated commit sha1. New commits:
b3f8dd2  Trac #26097: _directions_of_edges() doctests ...

comment:19 followup: 20 Changed 4 years ago by
b3f8dd2
doctest each dict separately with a pprint
call
@embray Do you know a way to force sage to use pprint for dict __repr__
?
just a plain dict is generally not a problem because its repr is already pprinted in ipython
Maybe it's ipython version dependent. I will check that.
comment:20 Changed 4 years ago by
Replying to vklein:
just a plain dict is generally not a problem because its repr is already pprinted in ipython
Maybe it's ipython version dependent. I will check that.
I don't think that's it. I think it may just be that the doctests don't pass through the IPython output formatting. Perhaps they should.
comment:21 Changed 4 years ago by
Yes.
Or the problem can be that ipython's lib.pretty is not consistent between py2 and py3 :
In py3
sage: from IPython.lib.pretty import pprint sage: pprint(tails) {2: [1, 3, 2, 4], 1: [2, 3, 1, 4], 3: [1, 3, 2, 4], 4: [2, 3, 1, 4]} sage: from pprint import pprint sage: pprint(tails) {1: [2, 3, 1, 4], 2: [1, 3, 2, 4], 3: [1, 3, 2, 4], 4: [2, 3, 1, 4]}
In Py2
sage: from IPython.lib.pretty import pprint sage: pprint(tails) {1: [2, 3, 1, 4], 2: [1, 3, 2, 4], 3: [1, 3, 2, 4], 4: [2, 3, 1, 4]} sage: from pprint import pprint sage: pprint(tails) {1: [2, 3, 1, 4], 2: [1, 3, 2, 4], 3: [1, 3, 2, 4], 4: [2, 3, 1, 4]}
comment:22 Changed 4 years ago by
Reviewers:  → Frédéric Chapoton 

Status:  needs_review → positive_review 
ok, let it be. Merci !
comment:23 Changed 4 years ago by
Branch:  u/vklein/py3__some_fixes_in_knot_link_py → b3f8dd255b2adccb8c9a2b2bb6f148c6920bade5 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:24 Changed 4 years ago by
Commit:  b3f8dd255b2adccb8c9a2b2bb6f148c6920bade5 

I don't really like this as is, but okay.
I didn't realize that IPython wasn't using the standard pprint.pprint
. If its pprint
is broken on Python 3 we should fix that.
New commits:
Trac #26097: Some fixes in ``link.py`` for py3 :