Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26097 closed enhancement (fixed)

Py3: Some fixes in knot.link.py

Reported by: vklein Owned by:
Priority: major Milestone: sage-8.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:

Status badges

Description (last modified by vklein)

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 vklein

Summary: Py3: Some fix in knot.link.pyPy3: Some fixes in knot.link.py

comment:2 Changed 4 years ago by vklein

Branch: u/vklein/py3__some_fixes_in_knot_link_py

comment:3 Changed 4 years ago by vklein

Commit: 5a7aed4361ee41cc8df0fbf4242ffd42dcf3bd8b
Description: modified (diff)

New commits:

5a7aed4Trac #26097: Some fixes in ``link.py`` for py3 :

comment:4 Changed 4 years ago by git

Commit: 5a7aed4361ee41cc8df0fbf4242ffd42dcf3bd8b2d1ec036be5eed09981bfb30ff0da56c6f4b237e

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

2d1ec03Trac #26097: Fix some doctests in ``knot.py``

comment:5 Changed 4 years ago by vklein

Description: modified (diff)

comment:6 Changed 4 years ago by vklein

Status: newneeds_review

comment:7 Changed 4 years ago by Frédéric Chapoton

beware of #25768, closed but not yet in the latest beta..

comment:8 in reply to:  7 Changed 4 years ago by vklein

Replying to chapoton:

beware of #25768, closed but not yet in the latest beta..

Nice ! I was currently on the connected_components() problem ( and have not think of just avoiding the sort()).

That being said it look like these two tickets can be automatically merged.

comment:9 Changed 4 years ago by Frédéric Chapoton

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 vklein

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 in reply to:  9 Changed 4 years ago by Erik Bray

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 Changed 4 years ago by Erik Bray

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 2018-08-28-15-07-02-267ca96a.
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 in reply to:  12 Changed 4 years ago by vklein

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 Changed 4 years ago by Erik Bray

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 problem--in 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 in reply to:  14 Changed 4 years ago by vklein

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 git

Commit: 2d1ec036be5eed09981bfb30ff0da56c6f4b237e33c95fbf5ea95077e8d7f1d7f300556c7df034ba

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

fb171afTrac #26097: Some fixes in ``link.py`` for py3 :
33c95fbTrac #26097: Fix some doctests in ``knot.py``

comment:17 Changed 4 years ago by vklein

Rebased on 8.4.beta2

comment:18 Changed 4 years ago by git

Commit: 33c95fbf5ea95077e8d7f1d7f300556c7df034bab3f8dd255b2adccb8c9a2b2bb6f148c6920bade5

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

b3f8dd2Trac #26097: _directions_of_edges() doctests ...

comment:19 Changed 4 years ago by vklein

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 in reply to:  19 Changed 4 years ago by Erik Bray

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 vklein

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]}

Last edited 4 years ago by vklein (previous) (diff)

comment:22 Changed 4 years ago by Frédéric Chapoton

Reviewers: Frédéric Chapoton
Status: needs_reviewpositive_review

ok, let it be. Merci !

comment:23 Changed 4 years ago by Volker Braun

Branch: u/vklein/py3__some_fixes_in_knot_link_pyb3f8dd255b2adccb8c9a2b2bb6f148c6920bade5
Resolution: fixed
Status: positive_reviewclosed

comment:24 Changed 4 years ago by Erik Bray

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.

Note: See TracTickets for help on using tickets.