Opened 23 months ago
Closed 23 months ago
#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 )
various little fixes here and there...
Change History (17)
comment:1 Changed 23 months ago by
- Branch set to u/mantepse/py3__fix_findstat_interface_and_fancytuple
comment:2 Changed 23 months ago by
- 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
comment:3 follow-up: ↓ 6 Changed 23 months ago by
- 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: ↓ 5 Changed 23 months ago by
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
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: ↓ 7 Changed 23 months ago by
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: ↓ 8 Changed 23 months ago by
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
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: ↓ 10 Changed 23 months ago by
-
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): 411 411 sage: p = findstat({pi: pi.length() for pi in l}, depth=0); p # optional -- internet, random 412 412 0: (St000018: The number of inversions of a permutation., [], 873) 413 413 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
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): 411 411 sage: p = findstat({pi: pi.length() for pi in l}, depth=0); p # optional -- internet, random 412 412 0: (St000018: The number of inversions of a permutation., [], 873) 413 413 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
- Commit changed from 66931ff6189650c6b7c8a145978340dd24d627d2 to 66b263c709c0ae468916680b744fe18e843416a7
Branch pushed to git repo; I updated commit sha1. New commits:
66b263c | Merge branch 'develop' of git://trac.sagemath.org/sage into t/27346/py3__fix_findstat_interface_and_fancytuple
|
comment:12 Changed 23 months ago by
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
- Commit changed from 66b263c709c0ae468916680b744fe18e843416a7 to 488e74e8ddfa36757ca0479ea540778e8338f8b9
Branch pushed to git repo; I updated commit sha1. New commits:
488e74e | remove seemingly unnecessary python3 treatment
|
comment:14 Changed 23 months ago by
I don't get it, now it works without any special treatment.
comment:15 Changed 23 months ago by
works for me on python3
comment:16 Changed 23 months ago by
- 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
- Branch changed from u/mantepse/py3__fix_findstat_interface_and_fancytuple to 488e74e8ddfa36757ca0479ea540778e8338f8b9
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
make FindStat interface and FancyTuple python3 compatible