Opened 2 years ago

Closed 23 months ago

#23485 closed enhancement (fixed)

Ring morphisms should not override __richcmp__

Reported by: saraedum Owned by:
Priority: trivial Milestone: sage-8.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) 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 2 years ago by saraedum

  • Branch set to u/saraedum/ring_morphisms_should_not_override___richcmp__

comment:2 Changed 2 years ago by saraedum

  • Commit set to 2ee2abf22f8bf06995c2ce0fb70f365cba8055d8
  • Keywords sd87 added

New commits:

2ee2abfDo not override __richcmp__ in Ring morphisms

comment:3 Changed 2 years ago by saraedum

  • Status changed from new to needs_review

comment:4 Changed 2 years ago by saraedum

  • Cc roed added

comment:5 Changed 2 years ago by jdemeyer

  • 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 2 years ago by git

  • Commit changed from 2ee2abf22f8bf06995c2ce0fb70f365cba8055d8 to b6ef396f00e8cde354385ded5a9e36934c2016b9

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

b6ef396Remove obsolete type checks

comment:7 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:8 Changed 2 years ago by aly.deines

  • Reviewers set to Aly Deines
  • Status changed from needs_review to positive_review

Passes all doctests now too.

comment:9 Changed 2 years ago by jdemeyer

  • 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 2 years ago by jdemeyer (next)

comment:10 Changed 2 years ago by git

  • Commit changed from b6ef396f00e8cde354385ded5a9e36934c2016b9 to ffc7371eb9c3ff84e575cf3d31000ce3bd3ee2fd

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

ffc7371Revert "Remove obsolete type checks"

comment:11 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:12 Changed 2 years ago by roed

  • 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 23 months ago by vbraun

  • 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.