Opened 6 years ago
Closed 6 years ago
#19110 closed defect (fixed)
QQ(0)^1 raises SIGFPE (which is caught)
Reported by:  dkrenn  Owned by:  

Priority:  critical  Milestone:  sage6.9 
Component:  basic arithmetic  Keywords:  
Cc:  Merged in:  
Authors:  Benjamin Hackl  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  e63f5f3 (Commits, GitHub, GitLab)  Commit:  e63f5f3cbdb691803d1e77d217b3807c1f8e5ae2 
Dependencies:  Stopgaps: 
Description (last modified by )
sage: QQ(0)^(1) ... FloatingPointError: Floating point exception
This should be checked and raise a ZeroDivisionError
.
Change History (25)
comment:1 Changed 6 years ago by
 Type changed from PLEASE CHANGE to defect
comment:2 Changed 6 years ago by
 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 6 years ago by
 Summary changed from QQ(0)^1 crashes to QQ(0)^1 raises a FloatingPointError
comment:4 followup: ↓ 5 Changed 6 years ago by
comment:5 in reply to: ↑ 4 Changed 6 years ago by
 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 6 years ago by
 Summary changed from QQ(0)^1 crashes (but the crash is caught) to QQ(0)^1 raises SIGFPE (which is caught)
comment:7 Changed 6 years ago by
What about something like the following?

src/sage/rings/rational.pyx
a b cdef class Rational(sage.structure.element.FieldElement): 2397 2397 mpz_pow_ui(mpq_numref(x.value), mpq_numref(_self.value), nn) 2398 2398 mpz_pow_ui(mpq_denref(x.value), mpq_denref(_self.value), nn) 2399 2399 # mpz_swap(mpq_numref(x.value), mpq_denref(x.value)) # still a segfault 2400 mpq_inv(x.value, x.value)2401 2400 sig_off() 2402 return x2401 return ~x 2403 2402 elif nn > 0: 2404 2403 sig_on() 2405 2404 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 6 years ago by
The ~x
trick would work but be much less efficient than the current code.
comment:9 Changed 6 years ago by
 Branch set to u/behackl/arithmetic/QQinversion
 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): 2329 2336 2330 2337 if nn < 0: 2331 2338 sig_on() 2339 if mpz_sgn(mpq_numref(_self.value)) == 0: 2340 sig_off() 2341 raise ZeroDivisionError('rational division by zero') 2332 2342 # mpz_pow_ui(mpq_denref(x.value), mpq_numref(_self.value), <unsigned long int>(nn)) 2333 2343 # mpz_pow_ui(mpq_numref(x.value), mpq_denref(_self.value), <unsigned long int>(nn)) 2334 2344 # 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)

comment:10 Changed 6 years ago by
 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 6 years ago by
You can move the check above sig_on()
, then there is no need for the extra sig_off()
.
comment:12 Changed 6 years ago by
 Commit changed from d815cd151e84b172baaa9b9a18888257e23af475 to fcf38257d8d61a154aa19e84a5063f0ba4d55e76
Branch pushed to git repo; I updated commit sha1. New commits:
fcf3825  move comparison before sig_on()

comment:13 Changed 6 years ago by
 Status changed from new to needs_review
Done  thanks for the suggestion! I think this might be ready for review now.
comment:14 Changed 6 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:15 Changed 6 years ago by
 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 6 years ago by
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 6 years ago by
 Commit changed from fcf38257d8d61a154aa19e84a5063f0ba4d55e76 to 70e7a0c8707c0b8012d1197005d8e096dcaa129b
comment:18 Changed 6 years ago by
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 6 years ago by
... all tests passed. positive_review
again?
comment:20 Changed 6 years ago by
No, if you bikeshed, at least do it correctly: exception messages should start with a lowercase letter. So if anything, Rational
should be replaced by rational
.
comment:21 Changed 6 years ago by
 Commit changed from 70e7a0c8707c0b8012d1197005d8e096dcaa129b to e63f5f3cbdb691803d1e77d217b3807c1f8e5ae2
Branch pushed to git repo; I updated commit sha1. New commits:
e63f5f3  ZeroDivisionError: Rational.. > rational..

comment:22 Changed 6 years ago by
... then let's do some bikeshedding. I'm currently testing if I missed something, will report back later.
comment:23 Changed 6 years ago by
Again, all tests passed.
comment:24 Changed 6 years ago by
 Status changed from needs_work to positive_review
comment:25 Changed 6 years ago by
 Branch changed from u/behackl/arithmetic/QQinversion to e63f5f3cbdb691803d1e77d217b3807c1f8e5ae2
 Resolution set to fixed
 Status changed from positive_review to closed
(I modified the title, as a 'crash' hints that Sage may exist when the command is run)