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

Rational(3)%Rational(-1) fails #9955

Closed
haraldschilly opened this issue Sep 20, 2010 · 8 comments
Closed

Rational(3)%Rational(-1) fails #9955

haraldschilly opened this issue Sep 20, 2010 · 8 comments

Comments

@haraldschilly
Copy link
Member

This is inconsistent

sage: Rational(3)%Rational(-1)
ZeroDivisionError: Inverse does not exist.

but

sage: 3%(-1)
0

Component: basic arithmetic

Author: André Apitzsch

Reviewer: John Cremona

Merged: sage-4.6.2.alpha4

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

@JohnCremona
Copy link
Member

comment:1

This is caused by the following simpler bug:


sage: a=Integer(3)
sage: b=Integer(-1)
sage: a.inverse_mod(b)
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
...

ZeroDivisionError: Inverse does not exist.

which is easy to fix. In sage.rings.integer.Integer.inverse_mod there is special case for modulus n=1 but not for -1. Either ass this special case, or replace n by abs(n).

@JohnCremona
Copy link
Member

comment:3

The patch looks right and I tested that it works (but did not yet test the whole library).

BUT you need to add a doctest to show that the bug has been fixed. There's a similar doctest in the same function, so just add something similar.

Then I'll look at it again.

@a-andre
Copy link

a-andre commented Jan 28, 2011

Attachment: trac_9955.patch.gz

@a-andre
Copy link

a-andre commented Jan 28, 2011

comment:4

doctest added

@a-andre
Copy link

a-andre commented Jan 28, 2011

Author: André Apitzsch

@JohnCremona
Copy link
Member

Reviewer: John Cremona

@JohnCremona
Copy link
Member

comment:5

Replying to @a-andre:

doctest added

Thanks! Positive review.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2011

Merged: sage-4.6.2.alpha4

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

5 participants