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

Failure with Python int modulo a rational #14870

Closed
tscrim opened this issue Jul 9, 2013 · 9 comments
Closed

Failure with Python int modulo a rational #14870

tscrim opened this issue Jul 9, 2013 · 9 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Jul 9, 2013

Some fun I came across:

sage: int(5) % QQ(2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-8ec0f9994ded> in <module>()
----> 1 int(Integer(5)) % QQ(Integer(2))

TypeError: Argument 'self' has incorrect type (expected sage.rings.rational.Rational, got int)

Note that this works:

sage: 5 % QQ(2)
1

CC: @vbraun @roed314

Component: basic arithmetic

Author: Travis Scrimshaw

Reviewer: Beth Malmskog

Merged: sage-5.12.beta0

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

@tscrim tscrim added this to the sage-5.12 milestone Jul 9, 2013
@tscrim tscrim self-assigned this Jul 9, 2013
@tscrim
Copy link
Collaborator Author

tscrim commented Jul 11, 2013

Author: Travis Scrimshaw

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 11, 2013

comment:1

Fixed. Thanks Volker and David for your insight.

@vbraun
Copy link
Member

vbraun commented Jul 11, 2013

comment:3

You shouldn't check the type exactly, derived classes of rational should be treated like a rational.

Also, the whole PY_* macro usage that you see in the sources is mostly historical, nowadays Cython understands isinstance(x, Rational) just fine and this is the preferred form of a type check.

@sagetrac-malmskog
Copy link
Mannequin

sagetrac-malmskog mannequin commented Jul 11, 2013

comment:4

We tried using decimals like 3.0, coerced rationals like QQ(3), non-reduced fractions like 4/2, complex-like values like 3+0*I, and absolute values of integers as moduli (second argument in function)--all of these were correctly interpreted as integers. We also found correct error messages for moduli that didn't make sense, for example mod(31/10, 10) produces a correct error message. Looks good!

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 12, 2013

Attachment: trac_14870-fix_int_mod_QQ-ts.patch.gz

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 12, 2013

comment:5

New version as per Volker's comment.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 12, 2013

comment:6

@sagetrac-malmskog, you'll need to write your real name as the reviewer.

@sagetrac-malmskog
Copy link
Mannequin

sagetrac-malmskog mannequin commented Jul 15, 2013

Reviewer: Beth Malmskog

@jdemeyer jdemeyer modified the milestones: sage-5.12, sage-5.11 Jul 21, 2013
@jdemeyer
Copy link
Contributor

jdemeyer commented Aug 2, 2013

Merged: sage-5.12.beta0

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