#9615 closed defect (fixed)
doctest failures with lcalc functions in 4.5.2.alpha1
Reported by: | ddrake | Owned by: | rishi |
---|---|---|---|
Priority: | blocker | Milestone: | sage-4.5.2 |
Component: | doctest coverage | Keywords: | lcalc |
Cc: | bober, craigcitro, cremona, mrubinst@…, mpatel, mvngu, rishi, ylchapuy, rbeezer | Merged in: | sage-4.5.2.rc0 |
Authors: | Rishikesh, Leif Leonhardy | Reviewers: | Dan Drake, Mitesh Patel |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Doctest failures in alpha1: https://groups.google.com/group/sage-release/msg/8807ed7073c6793f :
File "/scratch/scratch/schilly/sage/sage-4.5.2.alpha1/devel/sage/sage/ libs/lcalc/lcalc_Lfunction.pyx", line 780: sage: L.value(0.5) Expected: 0 Got: -1.28235854574334e-17 -----------------------------------------------
This is related to #5396.
Attachments (4)
Change History (20)
comment:1 Changed 11 years ago by
- Cc rishi added
comment:2 Changed 11 years ago by
- Cc mpatel added
Changed 11 years ago by
comment:3 Changed 11 years ago by
- Owner changed from mvngu to rishi
I did not check my email before creating a duplicate ticket and submitting a patch. Please close #9624
comment:4 Changed 11 years ago by
- Cc bober craigcitro cremona mrubinst@… mvngu ylchapuy added
- Status changed from new to needs_review
I'm changing the status to needs_review
. Is that OK?
Also, if I don't do it, someone should prepend the ticket number in the patch commit string.
Changed 11 years ago by
Ticket number and somewhat more general comment in commit string. Apply only this patch.
comment:5 follow-ups: ↓ 6 ↓ 9 ↓ 11 Changed 11 years ago by
The patch fixes the problem on a 32-bit Ubuntu Hardy VM which exhibits the doctest failure.
One question, though: is lcalc expected to return precisely zero, or a "noisy zero"? It seems unusual to me that software doing floating point calculations would return precisely zero on some platforms and a noisy zero on others.
If this kind of behavior is expected on 32-bit versus 64-bit machines, and if lcalc's answers are imprecise anyway, then I think this can get a positive review. (IOW, my ignorance about lcalc is what's keeping this from a positive review...)
comment:6 in reply to: ↑ 5 Changed 11 years ago by
Replying to ddrake:
[...] It seems unusual to me that software doing floating point calculations would return precisely zero on some platforms and a noisy zero on others.
If this kind of behavior is expected on 32-bit versus 64-bit machines, [...]
I can only imagine this is related to (unintentionally) different [default] rounding modes on different processors.
So this patch fixes the failing doctest (which is IMHO ok for the moment), but doesn't address its reason.
Unless the involved abs()
(or <
) is somehow broken, I can give this a positive review without applying the patch and actually testing it... ;-)
comment:7 follow-up: ↓ 8 Changed 11 years ago by
The patched doctest lacks an explanation/reference back to this ticket, which I think would be appropriate in this case.
Not surprisingly passes all (long) tests in sage/libs/lcalc/
on Ubuntu 9.04 x86 (Pentium 4 Prescott, gcc 4.3.3), where the doctest previously failed.
I remember there was a single machine reported on sage-release to have a different result than my two 32-bit machines previously had, on which the patch should perhaps be tested, too (but afair also with abs(...)
below 10^{-15}).
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 11 years ago by
- Cc rbeezer added
Replying to leif:
I remember there was a single machine reported on sage-release to have a different result than my two 32-bit machines previously had, on which the patch should perhaps be tested, too (but afair also with
abs(...)
below 10^{-15}).
[...] On 32-bit Ubuntu 10.04 on Pentium M I get the same results as Harald, though the non-zero value in the lcalc test is slightly different (~e-18). [...]
Rob
Rob, could you perhaps test this patch, too? (Though I don't expect it to fail on your machine either, since your result was even closer to zero.)
comment:9 in reply to: ↑ 5 Changed 11 years ago by
In lcalc, the function being tested returns a complex number (double precision). It is probably the way different processors do the computations, that the doctest failures are occurring. The earlier doctest passed on all the machines I have access to.
The calculation of L-function is pretty involved, you can see the papers which are referred to in the patch in #5396.
Replying to ddrake:
The patch fixes the problem on a 32-bit Ubuntu Hardy VM which exhibits the doctest failure.
One question, though: is lcalc expected to return precisely zero, or a "noisy zero"? It seems unusual to me that software doing floating point calculations would return precisely zero on some platforms and a noisy zero on others.
If this kind of behavior is expected on 32-bit versus 64-bit machines, and if lcalc's answers are imprecise anyway, then I think this can get a positive review. (IOW, my ignorance about lcalc is what's keeping this from a positive review...)
comment:10 in reply to: ↑ 8 Changed 11 years ago by
Same 32-bit system as before, applied ".2" patch. Then
sage -t -long "devel/sage-main/sage/libs/lcalc/lcalc_Lfunction.pyx" [3.3 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 3.3 seconds
Replying to leif:
Replying to leif:
I remember there was a single machine reported on sage-release to have a different result than my two 32-bit machines previously had, on which the patch should perhaps be tested, too (but afair also with
abs(...)
below 10^{-15}).
[...] On 32-bit Ubuntu 10.04 on Pentium M I get the same results as Harald, though the non-zero value in the lcalc test is slightly different (~e-18). [...]
Rob
Rob, could you perhaps test this patch, too? (Though I don't expect it to fail on your machine either, since your result was even closer to zero.)
comment:11 in reply to: ↑ 5 ; follow-up: ↓ 14 Changed 11 years ago by
Replying to ddrake:
The patch fixes the problem on a 32-bit Ubuntu Hardy VM which exhibits the doctest failure.
For what it's worth, it also fixes it on two different skynet machines which had the failure: cicero (x86-Linux-pentium4-fc) and iras (ia64-Linux-suse).
comment:12 follow-up: ↓ 15 Changed 11 years ago by
- Merged in set to sage-4.5.2.rc0
- Reviewers set to Dan Drake
- Status changed from needs_review to positive_review
Unless anyone objects, I'm changing the status to positive_review and merging
in 4.5.2.rc0. The latter, which someone should review, follows up on Leif's suggestion. Or should I exclude it?
comment:13 Changed 11 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
comment:14 in reply to: ↑ 11 Changed 11 years ago by
Replying to jhpalmieri:
Replying to ddrake:
The patch fixes the problem on a 32-bit Ubuntu Hardy VM which exhibits the doctest failure.
For what it's worth, it also fixes it on two different skynet machines which had the failure: cicero (x86-Linux-pentium4-fc) and iras (ia64-Linux-suse).
Does iras default to 32-bit builds? (I only know it's running a 64-bit SuSE.)
I ask this because Harald also saw this doctest failure on a (newer) 64-bit processor running a 32-bit OS (Core2 quad, Ubuntu 8.10 x86).
I think someone should track down if this difference is an lcalc "feature" or perhaps a Sage library interface issue (and in the latter case open a new ticket).
comment:15 in reply to: ↑ 12 ; follow-up: ↓ 16 Changed 11 years ago by
Replying to mpatel:
[...] which someone should review, follows up on Leif's suggestion. Or should I exclude it?
Well, I would have written
... # "noisy" zero on some platforms (see #9615)
But now it's too late...
Changed 11 years ago by
Use Leif's better note. Replaces previous version. Apply on top of 9615.2.patch
comment:16 in reply to: ↑ 15 Changed 11 years ago by
- Reviewers changed from Dan Drake to Dan Drake, Mitesh Patel
Replying to leif:
But now it's too late...
I've attached an update with Leif's better comment. I'll merge
into 4.5.2.rc0. I'm adding Leif as an author and me as a reviewer.
Just for the record: I get exactly the same number with
From sage-release: