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

clean _cmp_ in rational.pyx #28945

Closed
fchapoton opened this issue Jan 2, 2020 · 13 comments
Closed

clean _cmp_ in rational.pyx #28945

fchapoton opened this issue Jan 2, 2020 · 13 comments

Comments

@fchapoton
Copy link
Contributor

CC: @tscrim

Component: refactoring

Author: Frédéric Chapoton

Branch/Commit: ceb4c64

Reviewer: Travis Scrimshaw

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

@fchapoton fchapoton added this to the sage-9.1 milestone Jan 2, 2020
@fchapoton
Copy link
Contributor Author

Commit: 6b06817

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/28945

@fchapoton
Copy link
Contributor Author

New commits:

6b06817some cleanup in rational.pyx (including removal of `_cmp_`)

@fchapoton
Copy link
Contributor Author

comment:2

I am a bit surprised that it works, but it does.

@fchapoton
Copy link
Contributor Author

comment:3

broken in 9.1.b2, apparently

@tscrim
Copy link
Collaborator

tscrim commented Jan 28, 2020

comment:4

So it is something with how comparisons are done in the ring extensions. The only thing I can really guess for why removing Rational._cmp_ breaks things is because that is being called because of the RingExtensionElement.__getattr__ that pulls methods from the _backend, which then gets Rational._cmp_ that overrides the RingExtensionElement._richcmp_ call. Hence, the RingExtensionElement._richcmp_ never actually got used. Yet, even if that is the case, I don't understand why it is broken.

@mkoeppe
Copy link
Member

mkoeppe commented May 1, 2020

comment:5

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 May 1, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2020

Changed commit from 6b06817 to ceb4c64

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

af04177some cleanup in rational.pyx (including removal of `_cmp_`)
ceb4c64converting `__richcmp__` to _richcmp_

@fchapoton
Copy link
Contributor Author

comment:7

hurrah, green bot !

@tscrim
Copy link
Collaborator

tscrim commented Jun 22, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jun 22, 2020

comment:8

Huzzah!

@vbraun
Copy link
Member

vbraun commented Jun 24, 2020

Changed branch from u/chapoton/28945 to ceb4c64

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