Opened 4 years ago

Closed 4 years ago

#19110 closed defect (fixed)

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

Reported by: dkrenn Owned by:
Priority: critical Milestone: sage-6.9
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Benjamin Hackl Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: e63f5f3 (Commits) Commit: e63f5f3cbdb691803d1e77d217b3807c1f8e5ae2
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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

This should be checked and raise a ZeroDivisionError.

Change History (25)

comment:1 Changed 4 years ago by dkrenn

  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Priority changed from major to critical
  • Summary changed from inconsistency: invert ZZ(0) vs. QQ(0) to QQ(0)^-1 crashes

comment:3 Changed 4 years ago by ncohen

  • Summary changed from QQ(0)^-1 crashes to QQ(0)^-1 raises a FloatingPointError

comment:4 follow-up: Changed 4 years ago by ncohen

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

comment:5 in reply to: ↑ 4 Changed 4 years ago by jdemeyer

  • Summary changed from QQ(0)^-1 raises a FloatingPointError to QQ(0)^-1 crashes (but the crash is caught)

Replying to ncohen:

(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.

comment:6 Changed 4 years ago by jdemeyer

  • Summary changed from QQ(0)^-1 crashes (but the crash is caught) to QQ(0)^-1 raises SIGFPE (which is caught)

comment:7 Changed 4 years ago by behackl

What about something like the following?

  • src/sage/rings/rational.pyx

    a b cdef class Rational(sage.structure.element.FieldElement): 
    23972397            mpz_pow_ui(mpq_numref(x.value), mpq_numref(_self.value), -nn)
    23982398            mpz_pow_ui(mpq_denref(x.value), mpq_denref(_self.value), -nn)
    23992399            # mpz_swap(mpq_numref(x.value), mpq_denref(x.value)) # still a segfault
    2400             mpq_inv(x.value, x.value)
    24012400            sig_off()
    2402             return x
     2401            return ~x
    24032402        elif nn > 0:
    24042403            sig_on()
    24052404            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?

comment:8 Changed 4 years ago by jdemeyer

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

comment:9 Changed 4 years ago by behackl

  • Branch set to u/behackl/arithmetic/QQ-inversion
  • Commit set to 310bbba440fd94f298e6024edd889c68f08ed1c0

... 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...

  • src/sage/rings/rational.pyx

    a b cdef class Rational(sage.structure.element.FieldElement): 
    23292336
    23302337        if nn < 0:
    23312338            sig_on()
     2339            if mpz_sgn(mpq_numref(_self.value)) == 0:
     2340                sig_off()
     2341                raise ZeroDivisionError('rational division by zero')
    23322342            # mpz_pow_ui(mpq_denref(x.value), mpq_numref(_self.value), <unsigned long int>(-nn))
    23332343            # mpz_pow_ui(mpq_numref(x.value), mpq_denref(_self.value), <unsigned long int>(-nn))
    23342344            # The above causes segfaults, so swap after instead...

New commits:

3b6b436raise ZeroDivisionError when QQ(0) is taken to a negative power
310bbbaadd a doctest for QQ(0)^(-1)
Last edited 4 years ago by behackl (previous) (diff)

comment:10 Changed 4 years ago by git

  • Commit changed from 310bbba440fd94f298e6024edd889c68f08ed1c0 to d815cd151e84b172baaa9b9a18888257e23af475

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

d815cd1_self.value instead of x.value

comment:11 Changed 4 years ago by jdemeyer

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

comment:12 Changed 4 years ago by git

  • Commit changed from d815cd151e84b172baaa9b9a18888257e23af475 to fcf38257d8d61a154aa19e84a5063f0ba4d55e76

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

fcf3825move comparison before sig_on()

comment:13 Changed 4 years ago by behackl

  • Authors set to Benjamin Hackl
  • Status changed from new to needs_review

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

comment:14 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:15 Changed 4 years ago by dkrenn

  • Status changed from positive_review to needs_work
sage: 1/0
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
...
ZeroDivisionError: Rational division by zero

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

comment:16 Changed 4 years ago by dkrenn

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...

comment:17 Changed 4 years ago by git

  • Commit changed from fcf38257d8d61a154aa19e84a5063f0ba4d55e76 to 70e7a0c8707c0b8012d1197005d8e096dcaa129b

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

d4c7263rational division --> Rational division
70e7a0cmore rational div.. --> Rational div..

comment:18 Changed 4 years ago by behackl

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.

comment:19 Changed 4 years ago by behackl

... all tests passed. positive_review again?

comment:20 Changed 4 years ago by jdemeyer

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.

comment:21 Changed 4 years ago by git

  • Commit changed from 70e7a0c8707c0b8012d1197005d8e096dcaa129b to e63f5f3cbdb691803d1e77d217b3807c1f8e5ae2

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

e63f5f3ZeroDivisionError: Rational.. --> rational..

comment:22 Changed 4 years ago by behackl

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

comment:23 Changed 4 years ago by behackl

Again, all tests passed.

comment:24 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to positive_review

comment:25 Changed 4 years ago by vbraun

  • Branch changed from u/behackl/arithmetic/QQ-inversion to e63f5f3cbdb691803d1e77d217b3807c1f8e5ae2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.