Opened 5 years ago
Closed 5 years ago
#23485 closed enhancement (fixed)
Ring morphisms should not override __richcmp__
Reported by:  Julian Rüth  Owned by:  

Priority:  trivial  Milestone:  sage8.1 
Component:  refactoring  Keywords:  sd87 
Cc:  David Roe  Merged in:  
Authors:  Julian Rüth  Reviewers:  Aly Deines, Jeroen Demeyer, David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  ffc7371 (Commits, GitHub, GitLab)  Commit:  ffc7371eb9c3ff84e575cf3d31000ce3bd3ee2fd 
Dependencies:  Stopgaps: 
Description
But only override _richcmp_
instead.
Overwriting __richcmp__
used to be necessary to override __hash__
. This is not the case anymore it seems.
Change History (13)
comment:1 Changed 5 years ago by
Branch:  → u/saraedum/ring_morphisms_should_not_override___richcmp__ 

comment:2 Changed 5 years ago by
Commit:  → 2ee2abf22f8bf06995c2ce0fb70f365cba8055d8 

Keywords:  sd87 added 
comment:3 Changed 5 years ago by
Status:  new → needs_review 

comment:4 Changed 5 years ago by
Cc:  David Roe added 

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

These blocks of code should go away:
if not isinstance(other, RingHomomorphism_cover): return (op == Py_NE)
I disagree with them in the first place and they are just pointless now.
comment:6 Changed 5 years ago by
Commit:  2ee2abf22f8bf06995c2ce0fb70f365cba8055d8 → b6ef396f00e8cde354385ded5a9e36934c2016b9 

Branch pushed to git repo; I updated commit sha1. New commits:
b6ef396  Remove obsolete type checks

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

comment:8 Changed 5 years ago by
Reviewers:  → Aly Deines 

Status:  needs_review → positive_review 
Passes all doctests now too.
comment:9 Changed 5 years ago by
Reviewers:  Aly Deines → Aly Deines, Jeroen Demeyer 

Status:  positive_review → needs_work 
Sorry, I was wrong. At least some of the type checks are still needed, because equal parents does not imply equal types here. For example:
sage: R.<x,y> = PolynomialRing(QQ, 2) sage: S.<a,b> = R.quo(x^2 + y^2) sage: phi = S.cover() sage: alpha = R.hom(R, (0,0)) sage: psi = phi.pre_compose(alpha) sage: parent(psi) is parent(phi) True sage: type(psi) is type(phi) True
Version 0, edited 5 years ago
by
(next)
comment:10 Changed 5 years ago by
Commit:  b6ef396f00e8cde354385ded5a9e36934c2016b9 → ffc7371eb9c3ff84e575cf3d31000ce3bd3ee2fd 

Branch pushed to git repo; I updated commit sha1. New commits:
ffc7371  Revert "Remove obsolete type checks"

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

comment:12 Changed 5 years ago by
Reviewers:  Aly Deines, Jeroen Demeyer → Aly Deines, Jeroen Demeyer, David Roe 

Status:  needs_review → positive_review 
Looks good to me.
comment:13 Changed 5 years ago by
Branch:  u/saraedum/ring_morphisms_should_not_override___richcmp__ → ffc7371eb9c3ff84e575cf3d31000ce3bd3ee2fd 

Resolution:  → fixed 
Status:  positive_review → closed 
Note: See
TracTickets for help on using
tickets.
New commits:
Do not override __richcmp__ in Ring morphisms