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: sage-8.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:

Status badges

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 Julian Rüth

Branch: u/saraedum/ring_morphisms_should_not_override___richcmp__

comment:2 Changed 5 years ago by Julian Rüth

Commit: 2ee2abf22f8bf06995c2ce0fb70f365cba8055d8
Keywords: sd87 added

New commits:

2ee2abfDo not override __richcmp__ in Ring morphisms

comment:3 Changed 5 years ago by Julian Rüth

Status: newneeds_review

comment:4 Changed 5 years ago by Julian Rüth

Cc: David Roe added

comment:5 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_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 git

Commit: 2ee2abf22f8bf06995c2ce0fb70f365cba8055d8b6ef396f00e8cde354385ded5a9e36934c2016b9

Branch pushed to git repo; I updated commit sha1. New commits:

b6ef396Remove obsolete type checks

comment:7 Changed 5 years ago by Julian Rüth

Status: needs_workneeds_review

comment:8 Changed 5 years ago by Alyson Deines

Reviewers: Aly Deines
Status: needs_reviewpositive_review

Passes all doctests now too.

comment:9 Changed 5 years ago by Jeroen Demeyer

Reviewers: Aly DeinesAly Deines, Jeroen Demeyer
Status: positive_reviewneeds_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 Jeroen Demeyer (next)

comment:10 Changed 5 years ago by git

Commit: b6ef396f00e8cde354385ded5a9e36934c2016b9ffc7371eb9c3ff84e575cf3d31000ce3bd3ee2fd

Branch pushed to git repo; I updated commit sha1. New commits:

ffc7371Revert "Remove obsolete type checks"

comment:11 Changed 5 years ago by Julian Rüth

Status: needs_workneeds_review

comment:12 Changed 5 years ago by David Roe

Reviewers: Aly Deines, Jeroen DemeyerAly Deines, Jeroen Demeyer, David Roe
Status: needs_reviewpositive_review

Looks good to me.

comment:13 Changed 5 years ago by Volker Braun

Branch: u/saraedum/ring_morphisms_should_not_override___richcmp__ffc7371eb9c3ff84e575cf3d31000ce3bd3ee2fd
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.