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

make divmod() work for more rings #33704

Closed
yyyyx4 opened this issue Apr 13, 2022 · 10 comments
Closed

make divmod() work for more rings #33704

yyyyx4 opened this issue Apr 13, 2022 · 10 comments

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Apr 13, 2022

Python's divmod() is only supported for EuclideanDomainElements in Sage, whereas .quo_rem() is more universal.

It is worth noting that

  • some rings implement .quo_rem(), //, % despite not being Euclidean domains (example: ZZ['x']); and
  • not even all elements of Euclidean domains are instances of EuclideanDomainElement (example: QQ['x']).
    Therefore, divmod() will often not work when it should.

This patch moves .__divmod__() up from EuclideanDomainElement to RingElement, which makes divmod() work for more rings. Of course, not all rings have division with remainder implemented, but we can simply let that fail in the callee.

Short mailing list discussion: https://groups.google.com/g/sage-devel/c/GlTlFkQCQ0M

CC: @roed314 @kcrisman

Component: algebra

Author: Lorenz Panny

Branch/Commit: 778bef3

Reviewer: Travis Scrimshaw

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

@yyyyx4 yyyyx4 added this to the sage-9.7 milestone Apr 13, 2022
@yyyyx4
Copy link
Member Author

yyyyx4 commented Apr 24, 2022

Branch: public/move_divmod_up_to_RingElement

@yyyyx4

This comment has been minimized.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Apr 24, 2022

Commit: 778bef3

@yyyyx4
Copy link
Member Author

yyyyx4 commented Apr 24, 2022

New commits:

778bef3move .__divmod__() up to RingElement

@yyyyx4 yyyyx4 changed the title support divmod() in addition to .quo_rem() make divmod() work for more rings Apr 24, 2022
@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2022

comment:3

Author name. ;) Once that is done, the patchbot will run. If that comes back green, then you can set a positive review.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Apr 25, 2022

comment:4

Oops! :)

@yyyyx4
Copy link
Member Author

yyyyx4 commented Apr 25, 2022

Author: Lorenz Panny

@yyyyx4
Copy link
Member Author

yyyyx4 commented Apr 25, 2022

comment:5

Bot seems green enough. Thanks!

@vbraun
Copy link
Member

vbraun commented May 22, 2022

Changed branch from public/move_divmod_up_to_RingElement to 778bef3

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

3 participants