#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: |
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)
Change History (19)
comment:1 Changed 9 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 9 years ago by
Component: | PLEASE CHANGE → numerical |
---|
comment:3 Changed 9 years ago by
Status: | needs_review → needs_work |
---|
comment:4 Changed 9 years ago by
Reviewers: | → Ben Hutz |
---|
comment:5 Changed 9 years ago by
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
Cc: | Adam Towsley added |
---|
comment:7 follow-up: 11 Changed 9 years ago by
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
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
Status: | needs_work → needs_review |
---|
comment:11 Changed 9 years ago by
Status: | positive_review → needs_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
Status: | needs_work → needs_review |
---|
I have added a doctest that explains the behavior for NaN.
comment:13 Changed 9 years ago by
Status: | needs_review → positive_review |
---|
Ok. This still looks fine and now we have the doctests.
comment:14 Changed 9 years ago by
Status: | positive_review → needs_work |
---|---|
Work issues: | doctest |
You should add a meaningful commit message (use hg qrefresh -e
for that)
Changed 9 years ago by
Attachment: | trac_15388_nan_fix.patch added |
---|
Fixes the behavior of log(NaN) in real and complex fields
comment:16 Changed 9 years ago by
Status: | needs_review → positive_review |
---|
ok. still looks good, but with a commit message this time.
comment:17 Changed 9 years ago by
Merged in: | → sage-5.13.beta3 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:18 Changed 9 years ago by
Reviewers: | Ben Hutz → Benjamin Hutz |
---|
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