Opened 5 years ago
Closed 4 years ago
#23485 closed enhancement (fixed)
Ring morphisms should not override __richcmp__
Reported by:  saraedum  Owned by:  

Priority:  trivial  Milestone:  sage8.1 
Component:  refactoring  Keywords:  sd87 
Cc:  roed  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 set to u/saraedum/ring_morphisms_should_not_override___richcmp__
comment:2 Changed 5 years ago by
 Commit set to 2ee2abf22f8bf06995c2ce0fb70f365cba8055d8
 Keywords sd87 added
comment:3 Changed 5 years ago by
 Status changed from new to needs_review
comment:4 Changed 5 years ago by
 Cc roed added
comment:5 Changed 5 years ago by
 Status changed from needs_review to 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 changed from 2ee2abf22f8bf06995c2ce0fb70f365cba8055d8 to 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 changed from needs_work to needs_review
comment:8 Changed 5 years ago by
 Reviewers set to Aly Deines
 Status changed from needs_review to positive_review
Passes all doctests now too.
comment:9 Changed 5 years ago by
 Reviewers changed from Aly Deines to Aly Deines, Jeroen Demeyer
 Status changed from positive_review to 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 changed from b6ef396f00e8cde354385ded5a9e36934c2016b9 to 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 changed from needs_work to needs_review
comment:12 Changed 5 years ago by
 Reviewers changed from Aly Deines, Jeroen Demeyer to Aly Deines, Jeroen Demeyer, David Roe
 Status changed from needs_review to positive_review
Looks good to me.
comment:13 Changed 4 years ago by
 Branch changed from u/saraedum/ring_morphisms_should_not_override___richcmp__ to ffc7371eb9c3ff84e575cf3d31000ce3bd3ee2fd
 Resolution set to fixed
 Status changed from positive_review to closed
Note: See
TracTickets for help on using
tickets.
New commits:
Do not override __richcmp__ in Ring morphisms