Opened 8 years ago
Closed 7 years ago
#11672 closed enhancement (fixed)
Some more doctests from the book "Calcul mathématique avec Sage"
Reported by: | mmezzarobba | Owned by: | mvngu |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.7 |
Component: | doctest coverage | Keywords: | |
Cc: | malb, Bouillaguet | Merged in: | sage-5.7.beta2 |
Authors: | Marc Mezzarobba | Reviewers: | Martin Albrecht, Charles Bouillaguet |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
The attached patch adds to the Sage testsuite most examples appearing Chapters 6 and 8 (Polynomials, Polynomial Systems) of the French book Calcul mathématique avec Sage. See #9395 for some background.
All tests pass with sage 5.5.
Attachments (1)
Change History (28)
comment:1 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Cc malb added
comment:3 Changed 8 years ago by
- Reviewers set to Martin Albrecht
- Status changed from needs_review to needs_work
Sure, I can take a look:
- Please rename the patch to have the ".patch" suffix.
- There might be an issue with the UTF-8, but we should try and wait if anybody complaints.
- Do you really want to include an autogenerated file without any manual post processing? ("This file was *autogenerated* from sagebook.tex with sagetex.sty")
- Please add your full name to the author field here on Trac.
- I get some numerical noise errors:
File "/mnt/usb1/scratch/malb/sage-4.7.2/devel/sage/sage/tests/french_book/mpoly.py", line 373: sage: [CDF['x'](p(y=ys[0][0])).roots() for p in J.gens()] Expected: [[(-0.6 - 1.30624677741e-16*I, 1), (0.6 + 1.30624677741e-16*I, 1)], [(0.6 - 3.13499226579e-16*I, 1), (2.6 + 3.13499226579e-16*I, 1)]] Got: [[(-0.6 - 1.30628991909e-16*I, 1), (0.6 + 1.30628991909e-16*I, 1)], [(0.6 - 3.13509580582e-16*I, 1), (2.6 + 3.13509580582e-16*I, 1)]]
comment:4 Changed 8 years ago by
Martin, on which platform do you get numerical noise errors? With Sage 4.7 on a 64-bit Core 2, I get no difference.
Paul
comment:5 Changed 8 years ago by
This was on sage.math (sorry, should have mentioned).
comment:7 Changed 7 years ago by
thanks Charles!
Paul
comment:8 Changed 7 years ago by
- Priority changed from major to minor
- Status changed from needs_work to needs_review
comment:9 Changed 7 years ago by
Charles: Thanks!
Here is a new version of the patch, updated to the last draft of the book (which contains what should hopefully be the final version of the chapters in question...) and to Sage 5.5.
comment:10 Changed 7 years ago by
- Description modified (diff)
- Milestone changed from sage-5.6 to sage-5.7
- Reviewers changed from Martin Albrecht to Martin Albrecht, Charles Bouillaguet
- Status changed from needs_review to positive_review
Looks good to me !
comment:11 Changed 7 years ago by
- Status changed from positive_review to needs_work
On sage.math (Linux x86_64):
sage -t -force_lib devel/sage/sage/tests/french_book/mpoly.py ********************************************************************** File "/release/merger/sage-5.7.beta0/devel/sage-main/sage/tests/french_book/mpoly.py", line 366: sage: [CDF['x'](p(y=ys[0][0])).roots() for p in J.gens()] Expected: [[(-0.6 - 1.30624...e-16*I, 1), (0.6 + 1.30624...e-16*I, 1)], [(0.6 - 3.13499...e-16*I, 1), (2.6 + 3.13499...e-16*I, 1)]] Got: [[(-0.6 - 1.30628991909e-16*I, 1), (0.6 + 1.30628991909e-16*I, 1)], [(0.6 - 3.13509580582e-16*I, 1), (2.6 + 3.13509580582e-16*I, 1)]] **********************************************************************
comment:12 Changed 7 years ago by
- Status changed from needs_work to needs_review
I am unable to reproduce the failure you get on sage.math (though I am working on Linux x86_64 too), so I just reduced the number of digits taken into account in this test again.
comment:13 Changed 7 years ago by
- Status changed from needs_review to positive_review
I confirm that I ran the tests before giving the earlier positive review. It did work fine on my core i7-3615QM CPU running OS X 10.7.5...
The new patch also gets a positive review (the tests pass).
comment:14 Changed 7 years ago by
- Status changed from positive_review to needs_work
On bsd (OS X 10.6 x86_64):
sage -t --long -force_lib devel/sage/sage/tests/french_book/mpoly.py ********************************************************************** File "/Users/buildbot/build/sage/bsd-1/bsd_full/build/sage-5.7.alpha1/devel/sage-main/sage/tests/french_book/mpoly.py", line 373: sage: J.variety(RDF) Expected: [{y: 396340.89..., x: 26.61226...}] Got: [{y: 396340.890167, x: -54.9445477048}] **********************************************************************
On arando (Linux Ubuntu 12.04 i686):
sage -t --long -force_lib devel/sage/sage/tests/french_book/mpoly.py ********************************************************************** File "/var/lib/buildbot/build/sage/arando-1/arando_full/build/sage-5.7.alpha1/devel/sage-main/sage/tests/french_book/mpoly.py", line 373: sage: J.variety(RDF) Expected: [{y: 396340.89..., x: 26.61226...}] Got: [{y: 396340.890167, x: -46.7888621592}] **********************************************************************
comment:15 Changed 7 years ago by
I wonder why we get different results for x
. If all computations are based on IEEE 754, we should get the same (wrong) value. Is it possible to know which components of Sage are used for J.variety(RDF)
on each platform? On mine I get:
sage: get_systems('J.variety(RDF)') ['MPFI', 'Singular', 'numpy', 'ginac']
Since MPFI is based on MPFR, MPFI should be architecture independent, but I wonder if the same applies to Singular, numpy and ginac...
Paul
comment:16 follow-up: ↓ 19 Changed 7 years ago by
Paul: Could it be that whatever is used is compiled using extended precision on some platforms? Another strange thing, though, is that
sage: J.variety(Reals(p)) [{y: -1.00000001350726, x: -1929.60019649825}, {y: -0.999999986492719, x: 1929.62017010095}, {y: 396340.890166545, x: 10.3009522157620}]
finds 3 solutions for p as low as 39...
Anyway, this test doesn't make much sense, I shouldn't have included this test in the first place. Here is a new version of the patch without that test. I took the opportunity to ignore the last few significant digits in another numerical result that might change a bit in the future.
comment:17 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:18 follow-up: ↓ 20 Changed 7 years ago by
Paul: Could it be that whatever is used is compiled using extended precision on some platforms?
yes, or with a different rounding precision. Anyway, it might be worth tracking down this issue. Jeroen, is there a way to trace the calls to the different Sage components (MPFI, Singular, numpy, ginac)?
Paul
comment:19 in reply to: ↑ 16 ; follow-up: ↓ 21 Changed 7 years ago by
Replying to mmezzarobba:
Paul: Could it be that whatever is used is compiled using extended precision on some platforms? Another strange thing, though, is that
sage: J.variety(Reals(p)) [{y: -1.00000001350726, x: -1929.60019649825}, {y: -0.999999986492719, x: 1929.62017010095}, {y: 396340.890166545, x: 10.3009522157620}]finds 3 solutions for p as low as 39...
Anyway, this test doesn't make much sense, I shouldn't have included this test in the first place. Here is a new version of the patch without that test. I took the opportunity to ignore the last few significant digits in another numerical result that might change a bit in the future.
Well, the test obviously indicate that there is a problem somewhere, and I tend to believe that we should keep the test and track the problem.
Either your computation is ill-conditioned, and the test should be removed from Sage (and probably also from the book), or there is a component of sage that misbehaves. We should understand what the situation is, and act accordingly. These tests have been there for 1.5 years, so we are not in a hurry. Maybe we could compare intermediate results on different machines?
comment:20 in reply to: ↑ 18 Changed 7 years ago by
Replying to zimmerma:
Jeroen, is there a way to trace the calls to the different Sage components (MPFI, Singular, numpy, ginac)?
I don't think so. You could of course use gdb
and set suitable breakpoints...
comment:21 in reply to: ↑ 19 Changed 7 years ago by
Replying to Bouillaguet:
Either your computation is ill-conditioned, and the test should be removed from Sage (and probably also from the book), or there is a component of sage that misbehaves. We should understand what the situation is, and act accordingly.
Sorry for the lack of context. Yes, the computation is ill-conditioned, that's deliberate (see http://purple.lateralis.org/sagebook-r1014.pdf, p. 210-211), and all three results above are completely wrong (the correct result is x=6.305...).
As Paul noted, in an ideal world, this code should nontheless yield the same result everywhere, so it might have made sense to test that the result didn't change over time. But there are a number of reasonable explanations for what we observe, and (as far as I can tell) in Sage, floating-point results are usually interpreted as mere approximations that should agree with the correct answer "up to some numerical noise" rather than well-defined exact results. So I am in favor of dropping this testcase—unless the policy is that machine-precision floating-point computations should yield exactly the same result on all platforms.
(Of course, this doesn't mean we shouldn't try to understand exactly what is going on!)
comment:22 follow-up: ↓ 24 Changed 7 years ago by
Or, add
J.variety(RDF) # random
comment:23 Changed 7 years ago by
I have created #13980 to keep track of the numerical noise issue, so that we can make progress on this ticket independently.
Paul
comment:24 in reply to: ↑ 22 Changed 7 years ago by
Replying to jdemeyer:
Or, add
J.variety(RDF) # random
Is that better? (Given that many examples from the book are already excluded from the tests for various reasons.) Please tell me if you would like me to update the test file.
comment:25 Changed 7 years ago by
It was only a suggestion, do as you wish.
comment:26 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:27 Changed 7 years ago by
- Merged in set to sage-5.7.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
Martin, please could you review this? This will help our book to be more stable with future versions of Sage. Thank you in advance (as a coauthor of the book, I'm quite reluctant to review this).
Paul