QQ(0)^-1 raises SIGFPE (which is caught)

[dkrenn]

sage: QQ(0)^(-1)
...
FloatingPointError: Floating point exception

This should be checked and raise a ZeroDivisionError.

Component: basic arithmetic

Author: Benjamin Hackl

Branch/Commit: e63f5f3

Reviewer: Jeroen Demeyer

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

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1,17 +1,7 @@
-
-```
-sage: ZZ(0)^(-1)
-...
-ZeroDivisionError: Rational division by zero
-```
-vs.

sage: QQ(0)^(-1) ... FloatingPointError: Floating point exception

-which is inconsistent (the latter should also raise a `ZeroDivisionError`.
-
-FYI, `1/ZZ(0)` and `1/QQ(0)` both raise a `ZeroDivisionError`.
-
+This should be checked and raise a `ZeroDivisionError`.

[6bdad4c1-1e26-4f2f-a442-a01a2292c181]

comment:4

(I modified the title, as a 'crash' hints that Sage may exist when the command is run)

[jdemeyer]

comment:5

Replying to @nathanncohen:

(I modified the title, as a 'crash' hints that Sage may exist when the command is run)

I think "Sage crashes" is much closer to the reality than what you think.

Essentially, Sage does crash but the signal handling framework handles this crash and turns it into an exception.

[behackl]

comment:7

What about something like the following?

--- a/src/sage/rings/rational.pyx
+++ b/src/sage/rings/rational.pyx
@@ -2397,9 +2397,8 @@ cdef class Rational(sage.structure.element.FieldElement):
             mpz_pow_ui(mpq_numref(x.value), mpq_numref(_self.value), -nn)
             mpz_pow_ui(mpq_denref(x.value), mpq_denref(_self.value), -nn)
             # mpz_swap(mpq_numref(x.value), mpq_denref(x.value)) # still a segfault
-            mpq_inv(x.value, x.value)
             sig_off()
-            return x
+            return ~x
         elif nn > 0:
             sig_on()
             mpz_pow_ui(mpq_numref(x.value), mpq_numref(_self.value), nn)

Basically, thats similar to what happens in rings/integer.pyx (I think). With these changes, I have

sage: QQ(0)^(-1)
Traceback (most recent call last):
...
ZeroDivisionError: rational division by zero

... and all doctests in rings/rational.pyx pass as well. Any thoughts?

[jdemeyer]

comment:8

The ~x trick would work but be much less efficient than the current code.

[behackl]

comment:9

... well, that makes sense. The following approach (pushed to the branch attached to this ticket) should be better -- however, I really don't know if thats the best way to check if the fraction is 0...

--- a/src/sage/rings/rational.pyx
+++ b/src/sage/rings/rational.pyx
@@ -2329,6 +2336,9 @@ cdef class Rational(sage.structure.element.FieldElement):

         if nn < 0:
             sig_on()
+            if mpz_sgn(mpq_numref(_self.value)) == 0:
+                sig_off()
+                raise ZeroDivisionError('rational division by zero')
             # mpz_pow_ui(mpq_denref(x.value), mpq_numref(_self.value), <unsigned long int>(-nn))
             # mpz_pow_ui(mpq_numref(x.value), mpq_denref(_self.value), <unsigned long int>(-nn))
             # The above causes segfaults, so swap after instead...

---

New commits:

3b6b436	raise ZeroDivisionError when QQ(0) is taken to a negative power
310bbba	add a doctest for QQ(0)^(-1)

[behackl]

Commit: 310bbba

[behackl]

Branch: u/behackl/arithmetic/QQ-inversion

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 310bbba to d815cd1

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

d815cd1

_self.value instead of x.value

[jdemeyer]

comment:11

You can move the check above sig_on(), then there is no need for the extra sig_off().

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from d815cd1 to fcf3825

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

fcf3825

move comparison before sig_on()

[behackl]

Author: Benjamin Hackl

[behackl]

comment:13

Done -- thanks for the suggestion! I think this might be ready for review now.

[jdemeyer]

Reviewer: Jeroen Demeyer

[dkrenn]

comment:15

sage: 1/0
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
...
ZeroDivisionError: Rational division by zero

Make the following consistent: Rational (above) vs. rational (this patch).

[dkrenn]

comment:16

To make it more difficult to decide: Pure Python >>> 1/0 or sage: int(1)/int(0) gives lower case ZeroDivisionError: integer division or modulo by zero...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

d4c7263	rational division --> Rational division
70e7a0c	more rational div.. --> Rational div..

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from fcf3825 to 70e7a0c

[behackl]

comment:18

Well, in order to be consistent within rational.pyx, this and

sage: ~QQ(0)
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
...
ZeroDivisionError: rational division by zero

have to be changed. I did that with the last two commits. I think that other inconsistencies (e.g. SR(0)^(-1) and ~SR(0)) should be handled on a separate ticket. Regarding the inconsistency between Python and Sage: I don't feel that this is too dramatic, but if there is the wish to uniformize that, this should be done on a separate ticket, too.

I'll do a make ptestlong to check if I oversaw something and report back with the result later.

[behackl]

comment:19

... all tests passed. positive_review again?

[jdemeyer]

comment:20

No, if you bikeshed, at least do it correctly: exception messages should start with a lower-case letter. So if anything, Rational should be replaced by rational.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 70e7a0c to e63f5f3

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

e63f5f3

ZeroDivisionError: Rational.. --> rational..

[behackl]

comment:22

... then let's do some bikeshedding. I'm currently testing if I missed something, will report back later.

[behackl]

comment:23

Again, all tests passed.

[vbraun]

Changed branch from u/behackl/arithmetic/QQ-inversion to e63f5f3