#27346 closed enhancement (fixed)

py3: fix FindStat interface and FancyTuple

Reported by: mantepse Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: chapoton, embray Merged in:
Authors: Martin Rubey Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 488e74e (Commits) Commit: 488e74e8ddfa36757ca0479ea540778e8338f8b9
Dependencies: Stopgaps:

Description (last modified by mantepse)

various little fixes here and there...

Change History (17)

comment:1 Changed 23 months ago by mantepse

  • Branch set to u/mantepse/py3__fix_findstat_interface_and_fancytuple

comment:2 Changed 23 months ago by mantepse

  • Authors set to Martin Rubey
  • Cc chapoton added
  • Commit set to 66931ff6189650c6b7c8a145978340dd24d627d2
  • Component changed from PLEASE CHANGE to python3
  • Description modified (diff)
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

New commits:

66931ffmake FindStat interface and FancyTuple python3 compatible

comment:3 follow-up: Changed 23 months ago by chapoton

  • Cc embray added

This is wrong:

+        if sys.version_info[0] < 3 and isinstance(result, unicode):

The "unicode" keyword cannot be used in code that is compatible with both py2 and py3. And one should refrain from using sys.version_info everywhere.

comment:4 follow-up: Changed 23 months ago by chapoton

Why is this ticket needed at all ? All doctests pass with py3, if I am not mistaken.

comment:5 in reply to: ↑ 4 Changed 23 months ago by mantepse

Replying to chapoton:

Why is this ticket needed at all ? All doctests pass with py3, if I am not mistaken.

Unfortunately, not at all:

martin@convex63:~/sage-python3$ ./sage -t --optional=sage,internet src/sage/databases/findstat.py 
too many failed tests, not using stored timings
Running doctests with ID 2019-02-25-10-44-09-a715d7b7.
Git branch: develop
Using --optional=internet,memlimit,sage
Doctesting 1 file.
sage -t src/sage/databases/findstat.py
...
28 items had failures:
  11 of  16 in sage.databases.findstat
   6 of  11 in sage.databases.findstat.FindStat
   4 of  10 in sage.databases.findstat.FindStat.__call__
   2 of   6 in sage.databases.findstat.FindStatCollection
   1 of   3 in sage.databases.findstat.FindStatCollections.__iter__
   7 of   9 in sage.databases.findstat.FindStatCollections._element_constructor_
   2 of   3 in sage.databases.findstat.FindStatMap.description
   2 of   3 in sage.databases.findstat.FindStatMap.id
   2 of   3 in sage.databases.findstat.FindStatMap.id_str
   2 of   3 in sage.databases.findstat.FindStatMap.name
   1 of   3 in sage.databases.findstat.FindStatMaps
   3 of   5 in sage.databases.findstat.FindStatStatistic.__eq__
   2 of   4 in sage.databases.findstat.FindStatStatistic.__getitem__
   1 of   3 in sage.databases.findstat.FindStatStatistic.__init__
   3 of   4 in sage.databases.findstat.FindStatStatistic.__ne__
   2 of   4 in sage.databases.findstat.FindStatStatistic.__repr__
   1 of   9 in sage.databases.findstat.FindStatStatistic._find_by_values
   4 of   6 in sage.databases.findstat.FindStatStatistic._generating_functions_dict
   3 of   8 in sage.databases.findstat.FindStatStatistic._raise_error_modifying_statistic_with_perfect_match
   1 of   3 in sage.databases.findstat.FindStatStatistic.data
   3 of   5 in sage.databases.findstat.FindStatStatistic.first_terms
   1 of   2 in sage.databases.findstat.FindStatStatistic.function
   1 of   2 in sage.databases.findstat.FindStatStatistic.references
   3 of   4 in sage.databases.findstat.FindStatStatistic.set_code
   5 of   6 in sage.databases.findstat.FindStatStatistic.set_description
   3 of   4 in sage.databases.findstat.FindStatStatistic.set_references
   3 of   4 in sage.databases.findstat.FindStatStatistic.set_sage_code
   1 of   2 in sage.databases.findstat.FindStatStatistic.submit
    [247 tests, 80 failures, 16.24 s]
----------------------------------------------------------------------
sage -t src/sage/databases/findstat.py  # 80 doctests failed
----------------------------------------------------------------------
Total time for all tests: 16.3 seconds
    cpu time: 3.0 seconds
    cumulative wall time: 16.2 seconds

comment:6 in reply to: ↑ 3 ; follow-up: Changed 23 months ago by mantepse

Replying to chapoton:

This is wrong:

+        if sys.version_info[0] < 3 and isinstance(result, unicode):

The "unicode" keyword cannot be used in code that is compatible with both py2 and py3. And one should refrain from using sys.version_info everywhere.

I took this from #23475:

diff --git a/src/sage/structure/sage_object.pyx b/src/sage/structure/sage_object.pyx
index ca1227b..6a00995 100644
--- a/src/sage/structure/sage_object.pyx
+++ b/src/sage/structure/sage_object.pyx
@@ -235,8 +235,8 @@ cdef class SageObject:
             return str(type(self))
         else:
             result = repr_func()
-            if isinstance(result, unicode):
-                # Py3 compatibility: allow _repr_ to return unicode
+            if sys.version_info[0] < 3 and isinstance(result, unicode):
+                # for Py3 compatibility: allow _repr_ to return unicode
                 return result.encode('utf-8')
             else:
                 return result

comment:7 in reply to: ↑ 6 ; follow-up: Changed 23 months ago by chapoton

This is available only inside .pyx files, not for .py files

Replying to mantepse:

Replying to chapoton:

This is wrong:

+        if sys.version_info[0] < 3 and isinstance(result, unicode):

The "unicode" keyword cannot be used in code that is compatible with both py2 and py3. And one should refrain from using sys.version_info everywhere.

I took this from #23475:

diff --git a/src/sage/structure/sage_object.pyx b/src/sage/structure/sage_object.pyx
index ca1227b..6a00995 100644
--- a/src/sage/structure/sage_object.pyx
+++ b/src/sage/structure/sage_object.pyx
@@ -235,8 +235,8 @@ cdef class SageObject:
             return str(type(self))
         else:
             result = repr_func()
-            if isinstance(result, unicode):
-                # Py3 compatibility: allow _repr_ to return unicode
+            if sys.version_info[0] < 3 and isinstance(result, unicode):
+                # for Py3 compatibility: allow _repr_ to return unicode
                 return result.encode('utf-8')
             else:
                 return result

comment:8 in reply to: ↑ 7 Changed 23 months ago by mantepse

Strange, because it seems to work. Anyway, I have no idea how to fix it differently, can you help?

Replying to chapoton:

This is available only inside .pyx files, not for .py files

Replying to mantepse:

Replying to chapoton:

This is wrong:

+        if sys.version_info[0] < 3 and isinstance(result, unicode):

The "unicode" keyword cannot be used in code that is compatible with both py2 and py3. And one should refrain from using sys.version_info everywhere.

I took this from #23475:

diff --git a/src/sage/structure/sage_object.pyx b/src/sage/structure/sage_object.pyx
index ca1227b..6a00995 100644
--- a/src/sage/structure/sage_object.pyx
+++ b/src/sage/structure/sage_object.pyx
@@ -235,8 +235,8 @@ cdef class SageObject:
             return str(type(self))
         else:
             result = repr_func()
-            if isinstance(result, unicode):
-                # Py3 compatibility: allow _repr_ to return unicode
+            if sys.version_info[0] < 3 and isinstance(result, unicode):
+                # for Py3 compatibility: allow _repr_ to return unicode
                 return result.encode('utf-8')
             else:
                 return result

comment:9 follow-up: Changed 23 months ago by embray

  • src/sage/databases/findstat.py

    diff --git a/src/sage/databases/findstat.py b/src/sage/databases/findstat.py
    index 5bd5fff..e6af9e0 100644
    a b class FindStat(SageObject): 
    411411        sage: p = findstat({pi: pi.length() for pi in l}, depth=0); p           # optional -- internet, random
    412412        0: (St000018: The number of inversions of a permutation., [], 873)
    413413
    414     Note however, that the results of these two queries are not
    415     necessarily the same, because we compare queries by the data
    416     sent, and the ordering of the data might be different::
    417 
    418         sage: p == q                                                            # optional -- internet
    419         False
     414    Note however, that the results of these two queries need not
     415    compare equal, because we compare queries by the data
     416    sent, and the ordering of the data might be different.

Not that I understand what this example is demonstrating or whether it's important, but I also don't understand why it was removed in service to Python 3 fixes? Why shouldn't this example work on Python 3? Or is it an example that is not expected to work at all (or maybe if the example is random it might be better if it were kept but marked # random, emphasizing that the result may or may not be false).

comment:10 in reply to: ↑ 9 Changed 23 months ago by mantepse

I removed it, because with python3 it surprisingly gave True, but more importantly, because I think it is not very interesting. When I implemented all this, I was not sure how to structure the various results returned by findstat. I now know a little better, and I have plans for a much better interface, but not really the time right now.

If you insist, I can mark the doctest as random.

Replying to embray:

  • src/sage/databases/findstat.py

    diff --git a/src/sage/databases/findstat.py b/src/sage/databases/findstat.py
    index 5bd5fff..e6af9e0 100644
    a b class FindStat(SageObject): 
    411411        sage: p = findstat({pi: pi.length() for pi in l}, depth=0); p           # optional -- internet, random
    412412        0: (St000018: The number of inversions of a permutation., [], 873)
    413413
    414     Note however, that the results of these two queries are not
    415     necessarily the same, because we compare queries by the data
    416     sent, and the ordering of the data might be different::
    417 
    418         sage: p == q                                                            # optional -- internet
    419         False
     414    Note however, that the results of these two queries need not
     415    compare equal, because we compare queries by the data
     416    sent, and the ordering of the data might be different.

Not that I understand what this example is demonstrating or whether it's important, but I also don't understand why it was removed in service to Python 3 fixes? Why shouldn't this example work on Python 3? Or is it an example that is not expected to work at all (or maybe if the example is random it might be better if it were kept but marked # random, emphasizing that the result may or may not be false).

comment:11 Changed 23 months ago by git

  • Commit changed from 66931ff6189650c6b7c8a145978340dd24d627d2 to 66b263c709c0ae468916680b744fe18e843416a7

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

66b263cMerge branch 'develop' of git://trac.sagemath.org/sage into t/27346/py3__fix_findstat_interface_and_fancytuple

comment:12 Changed 23 months ago by chapoton

inside src/sage/databases/oeis.py, use bytes_to_str

available from

from sage.cpython.string import bytes_to_str

comment:13 Changed 23 months ago by git

  • Commit changed from 66b263c709c0ae468916680b744fe18e843416a7 to 488e74e8ddfa36757ca0479ea540778e8338f8b9

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

488e74eremove seemingly unnecessary python3 treatment

comment:14 Changed 23 months ago by mantepse

I don't get it, now it works without any special treatment.

comment:15 Changed 23 months ago by chapoton

works for me on python3

comment:16 Changed 23 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let it be.

comment:17 Changed 23 months ago by vbraun

  • Branch changed from u/mantepse/py3__fix_findstat_interface_and_fancytuple to 488e74e8ddfa36757ca0479ea540778e8338f8b9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.