Opened 6 years ago
Closed 5 years ago
#22297 closed enhancement (fixed)
py3 remove __cmp__ from Element
Reported by:  Frédéric Chapoton  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  python3  Keywords:  
Cc:  Travis Scrimshaw, Jeroen Demeyer, aapitzsch, John Palmieri  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  6124122 (Commits, GitHub, GitLab)  Commit:  61241224f0b95c91e6000108669de70cb2e827c6 
Dependencies:  Stopgaps: 
Description (last modified by )
as another step to py3
Change History (49)
comment:1 Changed 6 years ago by
Branch:  → u/chapoton/22297 

Commit:  → 5ad0b9b5468636871ccb73366263a5caac008dcf 
Status:  new → needs_review 
comment:2 Changed 6 years ago by
Status:  needs_review → needs_work 

This is obviously not correct: you cannot just remove that branch.
comment:4 Changed 6 years ago by
Cc:  Travis Scrimshaw Jeroen Demeyer aapitzsch John Palmieri added 

bot is green.
comment:5 followup: 6 Changed 6 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 Changed 6 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 6 years ago by
Authors:  Frédéric Chapoton → Jeroen Demeyer 

Description:  modified (diff) 
Summary:  py3 remove cmp() in element.pyx → py3 remove __cmp__ from Element 
If you really want to forward to Python 3, why not remove __cmp__
completely?
comment:8 Changed 6 years ago by
Branch:  u/chapoton/22297 → u/jdemeyer/22297 

comment:9 Changed 6 years ago by
Commit:  5ad0b9b5468636871ccb73366263a5caac008dcf → 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 6 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 6 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 followup: 14 Changed 6 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 6 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 followups: 16 17 Changed 6 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 followup: 21 Changed 6 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 Changed 6 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 Changed 6 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 6 years ago by
Commit:  01458896eb6cb1d79573b58c0f5fc74a0e644d15 → 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 6 years ago by
Branch:  u/jdemeyer/22297 

Commit:  a2310fc7da8d2c4e59789f9840eaabe294cf59dd 
Oops, wrong ticket...
comment:20 Changed 6 years ago by
Authors:  Jeroen Demeyer 

comment:21 Changed 6 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 6 years ago by
Branch:  → u/jdemeyer/22297 

Commit:  → 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 6 years ago by
Branch:  u/jdemeyer/22297 → public/22297 

Commit:  a2310fc7da8d2c4e59789f9840eaabe294cf59dd → 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:25 Changed 6 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 6 years ago by
Milestone:  sage7.6 → sage8.0 

so we probably really should fix #22257 first
comment:28 Changed 6 years ago by
Commit:  8ff1e7973267aa08986f0e645612f0bec17c74c3 → fe632c44f09d5df73438a66f9f501a2a0a0ca46e 

Branch pushed to git repo; I updated commit sha1. New commits:
fe632c4  Merge branch 'public/22297' in 8.0.b7

comment:29 Changed 6 years ago by
Commit:  fe632c44f09d5df73438a66f9f501a2a0a0ca46e → 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 5 years ago by
Commit:  d05cd3f214728effcbc0a7af73186b2401d14c83 → 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:32 Changed 5 years ago by
Commit:  7a7afb83f71076133e83d960d8234c8f566680ca → b6161114435c936a65bd7ce5b45b81627dff6fac 

Branch pushed to git repo; I updated commit sha1. New commits:
b616111  Merge branch 'public/22297' in 8.0.b10

comment:34 Changed 5 years ago by
Status:  needs_work → needs_review 

this should be ready, let us wait for a patchbot report
comment:36 Changed 5 years ago by
Authors:  → Frédéric Chapoton 

Status:  needs_review → 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 5 years ago by
Also, only this
left_cmp = left.__cmp__
should be inside the try
statement.
comment:38 Changed 5 years ago by
Commit:  b6161114435c936a65bd7ce5b45b81627dff6fac → 87ade9d5f8b3bac02cf13fca33d418b8ca56457d 

comment:39 Changed 5 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 5 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 5 years ago by
Commit:  87ade9d5f8b3bac02cf13fca33d418b8ca56457d → 0b3990339945037981e0975fcd61a9a28c1c3c8f 

Branch pushed to git repo; I updated commit sha1. New commits:
0b39903  trac 22297 without isinstance check

comment:42 Changed 5 years ago by
oh, I see. I did not know this syntax.
Let us try without the isinstance check then.
comment:43 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:44 Changed 5 years ago by
You don't need left_cmp = None
, just write pass
in the except
clause.
comment:45 Changed 5 years ago by
Commit:  0b3990339945037981e0975fcd61a9a28c1c3c8f → 61241224f0b95c91e6000108669de70cb2e827c6 

Branch pushed to git repo; I updated commit sha1. New commits:
6124122  trac 22297 detail

comment:48 Changed 5 years ago by
Reviewers:  → Jeroen Demeyer 

Status:  needs_review → positive_review 
comment:49 Changed 5 years ago by
Branch:  public/22297 → 61241224f0b95c91e6000108669de70cb2e827c6 

Resolution:  → fixed 
Status:  positive_review → closed 
let us wait for the bots
New commits:
py3: get rid of a spurious cmp() in element.pyx