Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#15388 closed defect (fixed)

log of NaN in RealField and ComplexField results in infinite loop

Reported by: Paul Fili Owned by: Paul Fili
Priority: minor Milestone: sage-5.13
Component: numerical Keywords: sage-days55
Cc: Adam Towsley Merged in: sage-5.13.beta3
Authors: Paul Fili, Adam Towsley Reviewers: Benjamin Hutz
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

If you have a RealField? or ComplexField? NaN value and attempt to compute log, the fact that NaN is considered < 0 results in RealField?.log calling ComplexField?.log, but then ComplexField?.log calls RealField?.log again, but again on the values NaN for the absolute value. This results in an infinite loop. Example code for the code:

x = RealField?()(NaN) x.log() # Results in infinite loop

This patch fixes the log function in RealField? and ComplexField? to return NaN if fed a number which is NaN (in either the real or the imaginary coordinate).

Attachments (1)

trac_15388_nan_fix.patch (1.7 KB) - added by Paul Fili 9 years ago.
Fixes the behavior of log(NaN) in real and complex fields

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by Paul Fili

Status: newneeds_review

comment:2 Changed 9 years ago by Paul Fili

Component: PLEASE CHANGEnumerical

comment:3 Changed 9 years ago by Ben Hutz

Status: needs_reviewneeds_work

A couple things

1) the patch needs a commit message

2) the patch name should be trac_15388_<description>.patch

3) Please add a doctest to each log function to demonstrate the fix

4) lines 2063, 2064 which are commented out look like they can just be removed as they add no value to the code

comment:4 Changed 9 years ago by Ben Hutz

Reviewers: Ben Hutz

comment:5 Changed 9 years ago by Ben Hutz

long doctest looks fine with this, so I'm happy with the code change, just make the minor changes suggested above.

Just to note, I had a few unrelated longtest failures on 5.12 that failed both with and without the patch.

sage -t --long /home/bhutz/sage/sage-5.12/devel/sage/sage/tests/cmdline.py  # 11 doctests failed
sage -t --long /home/bhutz/sage/sage-5.12/devel/sage/sage/calculus/desolvers.py  # 8 doctests failed
sage -t --long /home/bhutz/sage/sage-5.12/devel/sage/doc/en/constructions/calculus.rst  # 4 doctests failed
sage -t --long /home/bhutz/sage/sage-5.12/devel/sage/doc/en/prep/Quickstarts/Differential-Equations.rst  # 2 doctests failed
sage -t --long /home/bhutz/sage/sage-5.12/devel/sage/sage/misc/interpreter.py  # 1 doctest failed
sage -t --long /home/bhutz/sage/sage-5.12/devel/sage/sage/misc/trace.py  # 2 doctests failed
sage -t --long /home/bhutz/sage/sage-5.12/devel/sage/sage/tests/interrupt.pyx  # Time out

comment:6 Changed 9 years ago by Paul Fili

Cc: Adam Towsley added

comment:7 Changed 9 years ago by Paul Fili

It is probably best not to have an example involving NaN in the log's documentation, as it is such a basic function and this case is rather exceptional. (Although we still want it to work in case a log appears in someone's code after an NaN has already appeared, as happened to us when we found this bug.)

comment:8 Changed 9 years ago by Paul Fili

We have made the changes Ben suggested, except for adding a doctest, which seems undesirable as noted above.

comment:9 Changed 9 years ago by Paul Fili

Status: needs_workneeds_review

comment:10 Changed 9 years ago by Ben Hutz

Status: needs_reviewpositive_review

This looks good.

comment:11 in reply to:  7 Changed 9 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work
Work issues: doctest

Replying to paulfili:

Although we still want it to work in case a log appears in someone's code after an NaN has already appeared, as happened to us when we found this bug.

Which is exactly why a doctest is needed.

comment:12 Changed 9 years ago by Paul Fili

Status: needs_workneeds_review

I have added a doctest that explains the behavior for NaN.

comment:13 Changed 9 years ago by Ben Hutz

Status: needs_reviewpositive_review

Ok. This still looks fine and now we have the doctests.

comment:14 Changed 9 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work
Work issues: doctest

You should add a meaningful commit message (use hg qrefresh -e for that)

Changed 9 years ago by Paul Fili

Attachment: trac_15388_nan_fix.patch added

Fixes the behavior of log(NaN) in real and complex fields

comment:15 Changed 9 years ago by Paul Fili

Status: needs_workneeds_review

Commit message has been added.

comment:16 Changed 9 years ago by Ben Hutz

Status: needs_reviewpositive_review

ok. still looks good, but with a commit message this time.

comment:17 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.13.beta3
Resolution: fixed
Status: positive_reviewclosed

comment:18 Changed 9 years ago by Jeroen Demeyer

Reviewers: Ben HutzBenjamin Hutz
Note: See TracTickets for help on using tickets.