Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ring morphisms should not override __richcmp__ #23485

Closed
saraedum opened this issue Jul 19, 2017 · 16 comments
Closed

Ring morphisms should not override __richcmp__ #23485

saraedum opened this issue Jul 19, 2017 · 16 comments

Comments

@saraedum
Copy link
Member

But only override _richcmp_ instead.

Overwriting __richcmp__ used to be necessary to override __hash__. This is not the case anymore it seems.

CC: @roed314

Component: refactoring

Keywords: sd87

Author: Julian Rüth

Branch/Commit: ffc7371

Reviewer: Aly Deines, Jeroen Demeyer, David Roe

Issue created by migration from https://trac.sagemath.org/ticket/23485

@saraedum saraedum added this to the sage-8.1 milestone Jul 19, 2017
@saraedum
Copy link
Member Author

@saraedum
Copy link
Member Author

Changed keywords from none to sd87

@saraedum
Copy link
Member Author

Commit: 2ee2abf

@saraedum
Copy link
Member Author

New commits:

2ee2abfDo not override `__richcmp__` in Ring morphisms

@jdemeyer
Copy link

comment:5

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 20, 2017

Changed commit from 2ee2abf to b6ef396

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 20, 2017

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

b6ef396Remove obsolete type checks

@adeines
Copy link
Mannequin

adeines mannequin commented Jul 20, 2017

comment:8

Passes all doctests now too.

@adeines
Copy link
Mannequin

adeines mannequin commented Jul 20, 2017

Reviewer: Aly Deines

@jdemeyer
Copy link

comment:9

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)
False

@jdemeyer
Copy link

Changed reviewer from Aly Deines to Aly Deines, Jeroen Demeyer

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2017

Changed commit from b6ef396 to ffc7371

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2017

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

ffc7371Revert "Remove obsolete type checks"

@roed314
Copy link
Contributor

roed314 commented Jul 22, 2017

comment:12

Looks good to me.

@roed314
Copy link
Contributor

roed314 commented Jul 22, 2017

Changed reviewer from Aly Deines, Jeroen Demeyer to Aly Deines, Jeroen Demeyer, David Roe

@vbraun
Copy link
Member

vbraun commented Aug 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants