Opened 3 years ago
Closed 2 years ago
#26792 closed enhancement (fixed)
py3: Stop to use sort on KeyConvertingDict object
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Klein  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  c9c1047 (Commits, GitHub, GitLab)  Commit:  c9c10475a2dae3691bcfb330c99522df82e387b2 
Dependencies:  Stopgaps: 
Description
With python3 we currently have 40 failures of type
TypeError: '<' not supported between instances of 'KeyConvertingDict' and 'KeyConvertingDict'
These errors comes from the call of V.sort()
in multi_polynomial_ideal.py", line 2381, variety
function.
The root reason for calling sort
is for doctest displaying :
commit 17f42824d8941b23825fea1a6d82278bd5ef6c74 Author: Carl Witty <cwitty@…> Date: Wed Oct 24 18:36:37 2007 0700
Sort results of variety(), for better crossarchitecture doctest compatibility. This compensates for the fact that the results of Singular's triangular decomposition algorithm triangLfak appear in a different order on different architectures. (It would be better to fix this in triangular_decomposition(), but the obvious approach of sorting its result failed due to TRAC #991.)
As it will not make a lot of sense to implement comparison operators for KeyConvertingDict? this ticket aim at solving the sorting problems at doctests level rather than at python source code level.
Change History (34)
comment:1 Changed 3 years ago by
 Reviewers set to Vincent Klein
comment:2 Changed 3 years ago by
 Reviewers Vincent Klein deleted
comment:3 Changed 3 years ago by
 Branch set to u/vklein/26792
comment:4 Changed 3 years ago by
 Commit set to 4b8a3e59a962bb6c003162ff0ab8fe986c111d06
comment:5 Changed 3 years ago by
 Dependencies set to #23572
Add #23572 as dependency for mpoly renaming.
comment:6 Changed 3 years ago by
 Milestone changed from sage8.5 to sage8.7
comment:7 Changed 3 years ago by
 Commit changed from 4b8a3e59a962bb6c003162ff0ab8fe986c111d06 to b78d1c42b3d3dbab29075404285d965ded990ee6
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
b32170f  reviewer suggestion about independent set test

324b483  Merge branch 'public/french_book_python3' in 8.6.b0

a00c64c  livre sage doctests: fix floating errors

3c6139a  Merge branch 'public/french_book_python3' in 8.6.b1

6d6fb70  partial fix for numerical noise on osX

17d97d8  Merge branch 'public/french_book_python3' in 8.6

6da3629  book doctests : adding some tolerance again

5fd7867  Trac #26792: py3: Stop to use sort on KeyConvertingDict object

140623f  Trac #26792: py3: fix test.french_book_mpoly module

b78d1c4  Trac #26792: replace dict.iteritems calls by dict.items

comment:8 Changed 3 years ago by
 Commit changed from b78d1c42b3d3dbab29075404285d965ded990ee6 to ceee97f1c993abec0f194e23631ab799f86e71db
comment:10 Changed 3 years ago by
 Commit changed from ceee97f1c993abec0f194e23631ab799f86e71db to 3c0660cb0219f4d4f9a5f111050ddf652dc2d6ac
Branch pushed to git repo; I updated commit sha1. New commits:
3c0660c  Trac #26792: fix mpoly.py doctests

comment:11 Changed 3 years ago by
Fix mpoly.py doctests and remove #23572 dependency.
comment:12 Changed 3 years ago by
 Commit changed from 3c0660cb0219f4d4f9a5f111050ddf652dc2d6ac to 9f3984b12fba4c0ffec6ff761a5faf76637fad3b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9f3984b  Trac #26792: fix mpoly.py doctests

comment:13 Changed 3 years ago by
 Commit changed from 9f3984b12fba4c0ffec6ff761a5faf76637fad3b to f5ce2fa06f5f4c7da222461a8a3477d0302dbe75
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f5ce2fa  Trac #26792: fix mpoly.py doctests

comment:14 Changed 3 years ago by
 Dependencies set to #23572
ticket on hold in order to let priority for #23572
comment:15 Changed 2 years ago by
time to start again with this ticket ; this should be a great step forward when done
comment:16 Changed 2 years ago by
 Branch changed from u/vklein/26792 to public/ticket/26792
 Commit changed from f5ce2fa06f5f4c7da222461a8a3477d0302dbe75 to 2f908f0961293e36c42bd1e2fe81153fafd44b0b
 Dependencies #23572 deleted
 Status changed from new to needs_review
New commits:
2f908f0  Merge branch 'u/vklein/26792' in 8.7.b4

comment:17 Changed 2 years ago by
 Status changed from needs_review to needs_work
remains one failing doctest
comment:18 Changed 2 years ago by
 Commit changed from 2f908f0961293e36c42bd1e2fe81153fafd44b0b to f4a2bfa9f1490a4795ddf3d5232262d991325ae8
Branch pushed to git repo; I updated commit sha1. New commits:
f4a2bfa  trac 26792 fixing one doctest

comment:19 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:20 Changed 2 years ago by
I get a merge conflict with 8.7.beta4: src/sage/tests/books/computationalmathematicswithsagemath/mpoly_doctest.py
comment:21 Changed 2 years ago by
There is a related doctest failure in misc/converting_dict.py
. Four Python 3 failures in the develop branch are replaced by 1 with this branch.
comment:22 Changed 2 years ago by
 Commit changed from f4a2bfa9f1490a4795ddf3d5232262d991325ae8 to 945857ffe6b2f66ecbdeb84369ad0d402e51c5be
Branch pushed to git repo; I updated commit sha1. New commits:
945857f  trac 26792 trying to fix a doctest

comment:23 Changed 2 years ago by
The new test passes with py3 but not py2.
comment:24 followup: ↓ 27 Changed 2 years ago by
 Status changed from needs_review to needs_work
and before my last commit it was the contrary. Probably one should make sure that the repr of these "dicts" is conveniently sorted in the same way for both py2 and py3.
comment:25 Changed 2 years ago by
I will fix that.
comment:26 Changed 2 years ago by
 Commit changed from 945857ffe6b2f66ecbdeb84369ad0d402e51c5be to 59c980e59a8492da4e8088809dcbb2f102d38e23
Branch pushed to git repo; I updated commit sha1. New commits:
59c980e  Trac #26792: Fix doctest in converting_dict.py

comment:27 in reply to: ↑ 24 Changed 2 years ago by
Replying to chapoton:
and before my last commit it was the contrary. Probably one should make sure that the repr of these "dicts" is conveniently sorted in the same way for both py2 and py3.
The __repr__
called by KeyConvertingDict
is the one inherited from python's dict
i don't think we should override it just for doctests compatibility. I prefer to modify the doctests.
comment:28 followup: ↓ 30 Changed 2 years ago by
I was talking about the ipython representation. This is different (already for dict) from using str or repr. Using that in the repr would be better.
comment:29 Changed 2 years ago by
 Commit changed from 59c980e59a8492da4e8088809dcbb2f102d38e23 to 4dc152ad3be45d97b17669eccc9f943854c25368
Branch pushed to git repo; I updated commit sha1. New commits:
4dc152a  Trac #26792: Fix a doctest in multi_polynomial_ideal.py

comment:30 in reply to: ↑ 28 Changed 2 years ago by
Replying to chapoton:
I was talking about the ipython representation. This is different (already for dict) from using str or repr. Using that in the repr would be better.
I think you mean using Ipython pretty print. I don't know why it is like it's not called by default for KeyConvertingDict
as it is for dict
:
Adding some traces :
diff git a/src/sage/misc/converting_dict.py b/src/sage/misc/converting_dict.py index f9bdb17..f08e13f 100644  a/src/sage/misc/converting_dict.py +++ b/src/sage/misc/converting_dict.py @@ 140,6 +140,13 @@ class KeyConvertingDict(dict): key = self.key_conversion_function(key) return super(KeyConvertingDict, self).__setitem__(key, value) + def __repr__(self): + print("call __repr__") + return super(KeyConvertingDict, self).__repr__() + + def _repr_pretty_(self, p, cycle): + print("pretty") + def __delitem__(self, key): r""" Remove a mapping from the dictionary.
You get this:
sage: K.<x,y> = QQ[] sage: I = ideal([x^2+2*y5,x+y+3]) sage: v = I.variety(AA)[0]; v call __repr__ {x: 4.464101615137755?, y: 7.464101615137755?} sage: from IPython.lib.pretty import pprint sage: pprint(v) pretty
comment:31 Changed 2 years ago by
 Commit changed from 4dc152ad3be45d97b17669eccc9f943854c25368 to c9c10475a2dae3691bcfb330c99522df82e387b2
Branch pushed to git repo; I updated commit sha1. New commits:
c9c1047  Trac #26792: py3 fix doctests failing because ...

comment:32 Changed 2 years ago by
 Status changed from needs_work to needs_review
I fixed the remaining cases using #py2
#py3
flags as it seems to be the most readable solution right now.
There is still one failling test sort related to KeyConvertingDict
:
File "src/sage/tests/books/computationalmathematicswithsagemath/mpoly_doctest.py", line 409, in sage.tests.books.computationalmathematicswithsagemath.mpoly_doctest Failed example: J.variety(RealField(51)) Expected: [{y: 396340.890166545, x: 14.1660266425312}] Got: [{y: 396340.890166545, x: 35.0231212211684}]
The difference being the value of x i think this is out the scope of this ticket.
comment:33 Changed 2 years ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, let us move on here.
comment:34 Changed 2 years ago by
 Branch changed from public/ticket/26792 to c9c10475a2dae3691bcfb330c99522df82e387b2
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Trac #26781: replace dict.iteritems calls by dict.items