Opened 4 years ago
Closed 3 years ago
#22297 closed enhancement (fixed)
py3 remove __cmp__ from Element
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  python3  Keywords:  
Cc:  tscrim, jdemeyer, aapitzsch, jhpalmieri  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  6124122 (Commits)  Commit:  61241224f0b95c91e6000108669de70cb2e827c6 
Dependencies:  Stopgaps: 
Description (last modified by )
as another step to py3
Change History (49)
comment:1 Changed 4 years ago by
 Branch set to u/chapoton/22297
 Commit set to 5ad0b9b5468636871ccb73366263a5caac008dcf
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Status changed from needs_review to needs_work
This is obviously not correct: you cannot just remove that branch.
comment:3 followup: ↓ 5 Changed 4 years ago by
could you please explain why ? no doctest fail.
comment:5 in reply to: ↑ 3 ; followup: ↓ 6 Changed 4 years ago by
Replying to chapoton:
could you please explain why ? no doctest fail.
That's not how it works. You have to explain me why the change that you made makes sense. And "no doctest fail" is never a good explanation.
I'm not even saying that the change is wrong, but it certainly needs to be justified.
comment:6 in reply to: ↑ 5 Changed 4 years ago by
Replying to jdemeyer:
I'm not even saying that the change is wrong
Well, ok, I did say that, but that might have been too harsh.
comment:7 Changed 4 years ago by
 Description modified (diff)
 Summary changed from py3 remove cmp() in element.pyx to py3 remove __cmp__ from Element
If you really want to forward to Python 3, why not remove __cmp__
completely?
comment:8 Changed 4 years ago by
 Branch changed from u/chapoton/22297 to u/jdemeyer/22297
comment:9 Changed 4 years ago by
 Commit changed from 5ad0b9b5468636871ccb73366263a5caac008dcf to 01458896eb6cb1d79573b58c0f5fc74a0e644d15
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0145889  Remove generic __cmp__ from Element

comment:10 followup: ↓ 15 Changed 4 years ago by
This doesn't really work because there are Element classes which have different semantics for __cmp__
and __richcmp__
. So it's not easy to do this.
comment:11 followup: ↓ 12 Changed 4 years ago by
so, can I put back my branch and set it to needs review ?
it seems to me that what I remove can never happen (because this is a method of the element class, self is an Element)
comment:12 in reply to: ↑ 11 ; followup: ↓ 14 Changed 4 years ago by
Replying to chapoton:
so, can I put back my branch and set it to needs review ? it seems to me that what I remove can never happen (because this is a method of the element class, self is an Element)
I looked at your branch and it is wrong because it happens after coercion. There is no guarantee that the result of coercion will be Sage elements.
comment:13 Changed 4 years ago by
Anyway, I think it's a bad idea to try to remove those 3 lines. You should look at __cmp__
as a whole and fix that. It makes little sense to do a microchange like what you suggest.
comment:14 in reply to: ↑ 12 ; followups: ↓ 16 ↓ 17 Changed 4 years ago by
Replying to jdemeyer:
Replying to chapoton:
so, can I put back my branch and set it to needs review ? it seems to me that what I remove can never happen (because this is a method of the element class, self is an Element)
I looked at your branch and it is wrong because it happens after coercion. There is no guarantee that the result of coercion will be Sage elements.
oh ! really ? I thought that when things have a parent, they are elements. Would you have an example where this is not the case ?
Other possibilities that come to my mind : what would you think of replacing "cmp(left, right)
" by "left.__cmp__(right)
" (I have not yet tried if it works) ? or maybe by "raise TypeError('coercion should build members of Element class')
" ?
This is one of the latest calls to cmp() in a cython file. I would really like to be able to cythonize all the pxy files with python3.
comment:15 in reply to: ↑ 10 ; followup: ↓ 21 Changed 4 years ago by
Replying to jdemeyer:
This doesn't really work because there are Element classes which have different semantics for
__cmp__
and__richcmp__
. So it's not easy to do this.
Do you remember how many failures there are, and where ? how many classes are concerned by this bad double semantic ? I would suspect that this happens at least in symbolic ring and real numbers. This may also be the cause of the problems in #22257.
comment:16 in reply to: ↑ 14 Changed 4 years ago by
Replying to chapoton:
oh ! really ? I thought that when things have a parent, they are elements.
Of course. That's trivially true, since a parent is a concept which was invented for the Element
/Parent
classes.
Would you have an example where this is not the case ?
Not right away, but I know that it's possible.
Other possibilities that come to my mind : what would you think of replacing "
cmp(left, right)
" by "left.__cmp__(right)
"
I don't really like that, but this might be the least bad thing to do.
"
raise TypeError('coercion should build members of Element class')
" ?
Certainly not this.
comment:17 in reply to: ↑ 14 Changed 4 years ago by
Replying to chapoton:
oh ! really ? I thought that when things have a parent, they are elements. Would you have an example where this is not the case ?
OK, I have an example: the coercion of RealField(54)(1)
and float(1.0)
gives a float
.
This might be considered a bug though...
comment:18 Changed 4 years ago by
 Commit changed from 01458896eb6cb1d79573b58c0f5fc74a0e644d15 to a2310fc7da8d2c4e59789f9840eaabe294cf59dd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a2310fc  Replace parent_c > parent

comment:19 Changed 4 years ago by
 Branch u/jdemeyer/22297 deleted
 Commit a2310fc7da8d2c4e59789f9840eaabe294cf59dd deleted
Oops, wrong ticket...
comment:20 Changed 4 years ago by
comment:21 in reply to: ↑ 15 Changed 4 years ago by
Replying to chapoton:
Replying to jdemeyer:
This doesn't really work because there are Element classes which have different semantics for
__cmp__
and__richcmp__
. So it's not easy to do this.Do you remember how many failures there are, and where ?
I didn't really keep track. Certainly intervals too.
comment:22 Changed 4 years ago by
 Branch set to u/jdemeyer/22297
 Commit set to a2310fc7da8d2c4e59789f9840eaabe294cf59dd
putting Jeroen's branch back, so that one can run a bot on it
New commits:
a2310fc  Replace parent_c > parent

comment:23 Changed 4 years ago by
 Branch changed from u/jdemeyer/22297 to public/22297
 Commit changed from a2310fc7da8d2c4e59789f9840eaabe294cf59dd to 8ff1e7973267aa08986f0e645612f0bec17c74c3
trying with another branch, that just remove blankly __cmp__
this is a test branch, so that one can run a patchbot on it
New commits:
8ff1e79  test branch for removal of __cmp__

comment:24 Changed 4 years ago by
comment:25 Changed 4 years ago by
some of the problems (including the ones in Coxeter groups and heegner.py) reduces to the following one:
sage: A1=QuadraticField(5) sage: A2=QuadraticField(5) sage: A1==A2 False
or the existing doctest
QuadraticField(11, 'a') is QuadraticField(11, 'a')
In turn, this seems to come from problems in RealLazyField?.
from sage.rings.number_field.number_field import NumberFieldFactory nff = NumberFieldFactory("number_field_factory") sage: R.<x> = QQ[] a1=nff.create_object(None, (QQ, x^2 2, ('a',), RLF(sqrt(2)), None, None, False, None), check=False) a2=nff.create_object(None, (QQ, x^2 2, ('a',), RLF(sqrt(2)), None, None, False, None), check=False) a1 == a2
where the failure of equality comes from the RLF terms.
comment:26 Changed 4 years ago by
 Milestone changed from sage7.6 to sage8.0
so we probably really should fix #22257 first
comment:27 Changed 4 years ago by
comment:28 Changed 4 years ago by
 Commit changed from 8ff1e7973267aa08986f0e645612f0bec17c74c3 to fe632c44f09d5df73438a66f9f501a2a0a0ca46e
Branch pushed to git repo; I updated commit sha1. New commits:
fe632c4  Merge branch 'public/22297' in 8.0.b7

comment:29 Changed 4 years ago by
 Commit changed from fe632c44f09d5df73438a66f9f501a2a0a0ca46e to d05cd3f214728effcbc0a7af73186b2401d14c83
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d05cd3f  just remove __cmp__ completely in element.pyx

comment:30 Changed 3 years ago by
 Commit changed from d05cd3f214728effcbc0a7af73186b2401d14c83 to 7a7afb83f71076133e83d960d8234c8f566680ca
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7a7afb8  trying to remove __cmp__ from element.pyx

comment:31 Changed 3 years ago by
another preparation step is #23133
comment:32 Changed 3 years ago by
 Commit changed from 7a7afb83f71076133e83d960d8234c8f566680ca to b6161114435c936a65bd7ce5b45b81627dff6fac
Branch pushed to git repo; I updated commit sha1. New commits:
b616111  Merge branch 'public/22297' in 8.0.b10

comment:33 Changed 3 years ago by
maybe the last preparation step is #23273
comment:34 Changed 3 years ago by
 Status changed from needs_work to needs_review
this should be ready, let us wait for a patchbot report
comment:35 Changed 3 years ago by
ok, bot is essentially green. Please review.
comment:36 Changed 3 years ago by
 Status changed from needs_review to needs_work
Great news!
One detail: you should keep the doctest (changing cmp
and __cmp__
to rich comparisons accordingly). Perhaps move that doctest to _richcmp_
.
comment:37 Changed 3 years ago by
Also, only this
left_cmp = left.__cmp__
should be inside the try
statement.
comment:38 Changed 3 years ago by
 Commit changed from b6161114435c936a65bd7ce5b45b81627dff6fac to 87ade9d5f8b3bac02cf13fca33d418b8ca56457d
comment:39 Changed 3 years ago by
ok for the doctest. I also did something for the try statement, but was not sure of what you meant.
comment:40 Changed 3 years ago by
I meant something like
try: left_cmp = left.__cmp__ except AttributeError: pass else: if isinstance(left_cmp, MethodType): return left_cmp(right)
but I'm wondering if we still the check if isinstance(left_cmp, MethodType):
. Maybe this would work too:
try: left_cmp = left.__cmp__ except AttributeError: pass else: return left_cmp(right)
comment:41 Changed 3 years ago by
 Commit changed from 87ade9d5f8b3bac02cf13fca33d418b8ca56457d to 0b3990339945037981e0975fcd61a9a28c1c3c8f
Branch pushed to git repo; I updated commit sha1. New commits:
0b39903  trac 22297 without isinstance check

comment:42 Changed 3 years ago by
oh, I see. I did not know this syntax.
Let us try without the isinstance check then.
comment:43 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:44 Changed 3 years ago by
You don't need left_cmp = None
, just write pass
in the except
clause.
comment:45 Changed 3 years ago by
 Commit changed from 0b3990339945037981e0975fcd61a9a28c1c3c8f to 61241224f0b95c91e6000108669de70cb2e827c6
Branch pushed to git repo; I updated commit sha1. New commits:
6124122  trac 22297 detail

comment:46 Changed 3 years ago by
done, sorry
comment:47 Changed 3 years ago by
bot is morally green
EDIT now squarely green
comment:48 Changed 3 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:49 Changed 3 years ago by
 Branch changed from public/22297 to 61241224f0b95c91e6000108669de70cb2e827c6
 Resolution set to fixed
 Status changed from positive_review to closed
let us wait for the bots
New commits:
py3: get rid of a spurious cmp() in element.pyx