Opened 8 years ago
Closed 7 years ago
#11666 closed enhancement (fixed)
Upgrade MPFR to 3.1.0
Reported by: | leif | Owned by: | leif |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | packages: standard | Keywords: | sd36 sd32 MPFR spkg wishlist sd35.5 |
Cc: | jpflori, zimmerma | Merged in: | sage-5.0.beta7 |
Authors: | Mike Hansen, Jean-Pierre Flori, Volker Braun | Reviewers: | Paul Zimmermann, Jean-Pierre Flori, Volker Braun, Jeroen Demeyer |
Report Upstream: | Fixed upstream, but not in a stable release. | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12171, #12548 | Stopgaps: |
Description (last modified by )
Our current MPFR spkg is fairly old (based on MPFR 2.4.2), and upgrading it to the latest [stable] upstream release is now on the high-priority wishlist.
Use http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.p0.spkg
Apply:
Attachments (4)
Change History (94)
comment:1 Changed 8 years ago by
- Keywords sd32 added
comment:2 Changed 8 years ago by
- Cc jpflori added
comment:3 Changed 8 years ago by
- Description modified (diff)
- Owner changed from tbd to leif
- Summary changed from Upgrade MPFR to 3.0.1 + upstream patches to Upgrade MPFR to 3.0.1 + upstream patches or later (3.1.0)
comment:4 Changed 7 years ago by
- Description modified (diff)
comment:5 Changed 7 years ago by
- Description modified (diff)
The final MPFR 3.1.0 has been released today.
See the description for links.
comment:6 Changed 7 years ago by
- Dependencies set to 12171
- Summary changed from Upgrade MPFR to 3.0.1 + upstream patches or later (3.1.0) to Upgrade MPFR to 3.1.0
comment:7 Changed 7 years ago by
- Status changed from new to needs_review
I have an spkg at http://sage.math.washington.edu/mpfr-3.1.0.spkg. This is needed for flint2.
comment:8 Changed 7 years ago by
Shouldn't the dependency be the other way around? That is mpfi needs mpfr rather than mpfr needs mpfi? Have you tested this on a 32bit OS?
comment:9 Changed 7 years ago by
This version of MPFI should work with both versions of MPFR. However, the old version of MPFI will not work with the new version of MPFI.
I haven't tested it on a 32bit OS. This was primarily just so we could build flint2 in Sage.
comment:10 Changed 7 years ago by
- Cc zimmerma added
comment:11 follow-up: ↓ 13 Changed 7 years ago by
I wonder why comment 8 speaks about MPFI. Also, upstream (MPFR) we have seen several
problems with the --enable-thread-safe
option which is now enabled by default. Unless needed
for Sage, my advice would be to configure MPFR with --disable-thread-safe
.
Paul
comment:12 Changed 7 years ago by
- Status changed from needs_review to needs_info
the url from comment 7 does not work.
Paul
comment:13 in reply to: ↑ 11 Changed 7 years ago by
Replying to zimmerma:
I wonder why comment 8 speaks about MPFI. Also, upstream (MPFR) we have seen several problems with the
--enable-thread-safe
option which is now enabled by default. Unless needed for Sage, my advice would be to configure MPFR with--disable-thread-safe
.
I spoke about mpfi because this ticket is marked as depending on #12171 which is upgrading mpfi. My understanding of the dependency field is that it means that you need #12171 for this ticket. This is silly and should be the other way around.
I think comment 9 has not been written with a clear mind.
I asked about test on 32bit machine because of this [ https://github.com/cschwan/sage-on-gentoo/issues/13].
comment:14 Changed 7 years ago by
The spkg is actually at http://sage.math.washington.edu/home/mhansen/mpfr-3.1.0.spkg
Let me clarify the MPFI issue since I must have been too tired when I tried before. If we were to just install this version of MPFR, the current MPFI (1.3.4) will fail to build since it uses something which has been removed/moved in the new version of MPFR. If the new version of MPFI works with both the old and new versions of MPFR. So, if we want to actually include this package in Sage, we have to have the new version of MPFI in first. This is the reason I listed the MPFI ticket as a dependency of this one.
comment:15 Changed 7 years ago by
You are making sense now. I indeed remember making a patch for mpfi-1.4 to build against mpfr-3.x in gentoo, it would apply to 1.3.4 as well I think. Anyway there are a number of things to fix as far as I can see because of the inclusion of these two.
comment:16 Changed 7 years ago by
Some thoughts about the spkg-install script:
- It sets some CFLAGS and others itself, is it needed? does not mpfr look for the gmp header and pumps from there if possible? and should we take advantage of this (as in done for mpir where spkg-install look at what mpir chooses if CFLAGS et al. are empty)?
- Is the patch for Solaris stuff still needed? (see #6453) -> at least the code in mpn_exp.c did not change in mpfr 3.1.0, so I guess it has to be included to support outdated Solaris systems where memset is buggy?
- It always runs the test suite, whether SAGE_CHECK is set (then it is ran twice...) or not (then just once), is there still a good reason for doing so? It seems that such a behavior was also present in the MPIR spkg but was removed (see #9522 and #8664)
- make should be replaced by $MAKE in spkg-check
comment:17 Changed 7 years ago by
It sets some CFLAGS and others itself, is it needed?
as a developer of MPFR I strongly recommend to let MPFR choose the same CC and CFLAGS as GMP (they are read in gmp.h). Since we do this for MPFR, this greatly improved the portability of MPFR on different platforms with different compilers, in particuler it solves the ABI=32 vs ABI=64 issues.
Is the patch for Solaris stuff still needed?
I guess yes, since this was a bug in the Solaris kernel.
Paul
comment:18 follow-up: ↓ 19 Changed 7 years ago by
I've tested this spkg on top of #12171 on my 64-bit laptop (Core 2 Duo): all doctests pass.
Paul
comment:19 in reply to: ↑ 18 Changed 7 years ago by
comment:20 Changed 7 years ago by
- Keywords sd35.5 added
comment:21 follow-up: ↓ 27 Changed 7 years ago by
- Dependencies changed from 12171 to #12171
- Reviewers set to Paul Zimmermann
on a 32-bit Pentium 4, after applying #12171 and importing this spkg, I get when I start Sage:
ImportError: /localdisk/tmp/sage-4.7.2/local/lib/libmpfi.so.0: undefined symbol: mpfr_add_d Error importing ipy_profile_sage - perhaps you should run %upgrade? WARNING: Loading of ipy_profile_sage failed.
Maybe I should first import this spkg then that of MPFI?
Paul
comment:22 follow-up: ↓ 25 Changed 7 years ago by
Still about the spkg itself:
- Is there any reason to "unset RM"? it says it messes with libtool while running "make check". I do not see this restriction in the MPIR package, or the MPFI one e.g. Is this for a specific architecture? Is this really needed? I guess this is fixed by #3537
- MAKE is set back to make before running "make install" (and then "make check" in the original package), any undocumented reason?
I'll test all of this on my system but if someone can point me to the relevant info, I'd be grateful, Google wans not that helpful.
comment:23 follow-up: ↓ 24 Changed 7 years ago by
I've put an updated package implementing my remarks, mainly using the mpir spkg-install file, I've also updated the description and license info.
It can be found at
http://perso.telecom-paristech.fr/sage/mpfr-3.1.0.spkg
It seems to do what it should, and uccessfully built and rand MPFR test suite on a "usual" intel 64 bit system, not any idea if I broke anything for more exotic architectures. Did not ran sage test suite either.
comment:24 in reply to: ↑ 23 Changed 7 years ago by
Replying to jpflori:
It can be found at
I do get a 301 to http://perso.telecom-paristech.fr/~sage/mpfr-3.1.0.spkg and that gives a 404.
comment:25 in reply to: ↑ 22 Changed 7 years ago by
Replying to jpflori:
Still about the spkg itself:
- Is there any reason to "unset RM"? it says it messes with libtool while running "make check". I do not see this restriction in the MPIR package, or the MPFI one e.g. Is this for a specific architecture? Is this really needed? I guess this is fixed by #3537.
Unsetting RM
is safe, but meanwhile a bit redundant. Newer autotools use $RM
instead of rm -f
(or $RM -f
), so tend to fail if RM
happens to be set to (just) rm
.
- MAKE is set back to make before running "make install" (and then "make check" in the original package), any undocumented reason?
I'm pretty sure that's a relict from when people didn't know how to properly disable parallel make
. If in doubt, use $MAKE -j1 install
etc.
comment:26 Changed 7 years ago by
Sorry, this should be http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.spkg
comment:27 in reply to: ↑ 21 ; follow-up: ↓ 41 Changed 7 years ago by
Replying to zimmerma:
Maybe I should first import this spkg then that of MPFI?
Since MPFI depends on MPFR (and MPIR), you have to rebuild MPFI after upgrading one of the latter.
If you have enough time (and/or the machine is fast enough), you can copy new/updated spkgs into $SAGE_ROOT/spkg/standard/
and run
$ env SAGE_UPGRADING=yes make build
afterwards. That way all dependent packages automatically get rebuilt, which can take some time.
(Unfortunately there are also a couple of stupid transitive dependencies; e.g. ATLAS gets rebuilt if you update readline -- just because ATLAS now has an spkg-install
file written in Python, and Python depends on readline... So a lot of rebuilds performed by make
aren't really necessary.)
comment:28 follow-up: ↓ 39 Changed 7 years ago by
- Work issues set to fails on 32-bit
I tried importing the MPFI spkg+patches (#12171) and that spkg on top of 4.8.alpha6 on a 32-bit Pentium 4 and got the following failures:
sage -t "devel/sage-main/sage/matrix/matrix_mpolynomial_dense.pyx" sage -t "devel/sage-main/sage/matrix/constructor.py" sage -t "devel/sage-main/sage/matrix/matrix2.pyx" sage -t "devel/sage-main/sage/misc/randstate.pyx" sage -t "devel/sage-main/sage/modules/free_module_element.pyx" sage -t "devel/sage-main/sage/rings/real_mpfi.pyx" sage -t "devel/sage-main/sage/rings/complex_field.py" sage -t "devel/sage-main/sage/rings/complex_interval_field.py" sage -t "devel/sage-main/sage/rings/real_mpfr.pyx" # Killed/crashed sage -t "devel/sage-main/sage/schemes/elliptic_curves/ell_rational_field.py"
Paul
comment:29 Changed 7 years ago by
the failures are most likely due to the fact that the mpfr_urandom
and mpfr_urandomb
return identical values on processors with different word size in MPFR 3.1.0
(see http://www.mpfr.org/mpfr-3.1.0/#changes).
The bad news is that this will need changing the doctests.
The good news is that we won't need any more different doctests on 32-bit and 64-bit (assuming the MPIR random generator is independent of the word size).
Paul Zimmermann
comment:30 Changed 7 years ago by
For the record, Mike's spkg and my updated one both need to be rebased on top of #12131.
I'll take care of that later.
comment:31 Changed 7 years ago by
- Description modified (diff)
- Work issues changed from fails on 32-bit to failing doctests for 32 bits
You can finally find a rebased spkg where I discarded some stuff involving ABI which previously made sage print that it was building for 32 bits, whereas it actually had no effect, and which is rebased on #12131 at the usual address:
http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.spkg
I guess the issue with doctests is the last step before "needs review"
comment:32 Changed 7 years ago by
I guess my last package should be rebased on #12366 which has been merged into 5.beta4.
Paul, any news on the doctests problems ?
comment:33 Changed 7 years ago by
I will check the doctests on cicero (skynet) but I guess the issue is still there, since nothing was changed. I will try to make a patch (but since I'm in holidays, it might take some time).
Paul
comment:34 Changed 7 years ago by
I tried this patch (with dependencies) on top of Sage 4.8 and got several doctests failures:
sage -t devel/sage-11666/sage/schemes/elliptic_curves/ell_rational_field.py # 2 doctests failed sage -t devel/sage-11666/sage/schemes/elliptic_curves/heegner.py # 2 doctests failed sage -t devel/sage-11666/sage/matrix/matrix2.pyx # 3 doctests failed sage -t devel/sage-11666/sage/combinat/partition.py # 30 doctests failed sage -t devel/sage-11666/sage/calculus/calculus.py # 7 doctests failed sage -t devel/sage-11666/sage/misc/randstate.pyx # 13 doctests failed sage -t devel/sage-11666/sage/misc/misc.py # 1 doctests failed sage -t devel/sage-11666/sage/modules/free_module_element.pyx # 2 doctests failed sage -t devel/sage-11666/sage/rings/real_mpfr.pyx # 12 doctests failed sage -t devel/sage-11666/sage/matrix/constructor.py # 2 doctests failed sage -t devel/sage-11666/sage/combinat/sloane_functions.py # 5 doctests failed sage -t devel/sage-11666/sage/misc/cachefunc.py # 14 doctests failed sage -t devel/sage-11666/sage/combinat/combinat.py # 1 doctests failed sage -t devel/sage-11666/sage/libs/fplll/fplll.pyx # Killed/crashed sage -t devel/sage-11666/sage/combinat/species/species.py # 3 doctests failed sage -t devel/sage-11666/sage/combinat/species/sum_species.py # 3 doctests failed sage -t devel/sage-11666/sage/combinat/species/permutation_species.py # 3 doctests failed sage -t devel/sage-11666/sage/sets/disjoint_union_enumerated_sets.py # 8 doctests failed sage -t devel/sage-11666/sage/matrix/matrix_mpolynomial_dense.pyx # 2 doctests failed sage -t devel/sage-11666/sage/combinat/species/partition_species.py # 4 doctests failed sage -t devel/sage-11666/sage/combinat/species/product_species.py # 2 doctests failed sage -t devel/sage-11666/sage/rings/complex_field.py # 4 doctests failed sage -t devel/sage-11666/sage/rings/complex_interval_field.py # 3 doctests failed sage -t devel/sage-11666/sage/combinat/partitions.pyx # 1 doctests failed sage -t devel/sage-11666/sage/rings/real_mpfi.pyx # 5 doctests failed
Can someone reproduce that?
Paul
comment:35 Changed 7 years ago by
I've begun the test suite. Here are my failures on a Debian amd64 experimental system (on intel core i7) :
The following tests failed: sage -t -force_lib devel/sage/sage/modular/modsym/ambient.py # 0 doctests failed sage -t -force_lib devel/sage/sage/modular/hecke/hecke_operator.py # 0 doctests failed sage -t -force_lib devel/sage/sage/modular/hecke/ambient_module.py # 0 doctests failed sage -t -force_lib devel/sage/sage/modules/free_module_element.pyx # 2 doctests failed sage -t -force_lib devel/sage/sage/matrix/matrix_mpolynomial_dense.pyx # 2 doctests failed sage -t -force_lib devel/sage/sage/matrix/constructor.py # 2 doctests failed sage -t -force_lib devel/sage/sage/matrix/matrix2.pyx # 3 doctests failed sage -t -force_lib devel/sage/sage/rings/real_mpfi.pyx # 5 doctests failed sage -t -force_lib devel/sage/sage/rings/complex_field.py # 4 doctests failed sage -t -force_lib devel/sage/sage/rings/real_mpfr.pyx # 12 doctests failed sage -t -force_lib devel/sage/sage/rings/complex_interval_field.py # 3 doctests failed sage -t -force_lib devel/sage/sage/tests/cmdline.py # 4 doctests failed sage -t -force_lib devel/sage/sage/misc/randstate.pyx # 13 doctests failed sage -t -force_lib devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 2 doctests failed
I'll have a look at them tomorrow.
Some of them at least are just changes in random output (e. g. real_mpfr and real_mpfi)
comment:36 Changed 7 years ago by
Some other look more problematic:
sage -t -force_lib devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py *** Warning: new stack size = 1003360 (0.957 Mbytes). *** Warning: new stack size = 1003360 (0.957 Mbytes). Saturation index bound = 265 WARNING: saturation at primes p > 97 will not be done; points may be unsaturated at primes between 97 and index bound Failed to saturate MW basis at primes [ ] Saturation index bound = 265 WARNING: saturation at primes p > 199 will not be done; points may be unsaturated at primes between 199 and index bound Failed to saturate MW basis at primes [ ] After 10 attempts at enlargement, giving up! --points not proved 2-saturated, 2-saturation failed! Failed to saturate MW basis at primes [ 2 ] 2 After 10 attempts at enlargement, giving up! --points not proved 2-saturated, 2-saturation failed! Failed to saturate MW basis at primes [ 2 ]
Maybe because I did not rebuild Pari spkg?
Apart from running sage -b I just forced rebuild of libfplll before the above make ptest (because of lack of time).
I'll properly rebuild everything tomorrow and rerun the testsuite.
comment:37 Changed 7 years ago by
Nevermind, the above is potentially just verbosity and not related to the actual errors
File "/home/jp/boulot/sage/sage-4.8/devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py", line 2921: sage: 2 * E.elliptic_exponential(z) Expected: (2.04119347066305 - 1.10251372205166*I : 2.23105000976838 - 2.69795281735238*I : 1.00000000000000) Got: (-1.52184235874404 - 0.0581413944316544*I : 0.948655866506124 - 0.0381469928565030*I : 1.00000000000000) ********************************************************************** File "/home/jp/boulot/sage/sage-4.8/devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py", line 2923: sage: E.elliptic_exponential(2 * z) Expected: (2.04119347066305 - 1.10251372205167*I : 2.23105000976839 - 2.69795281735236*I : 1.00000000000000) Got: (-1.52184235874404 - 0.0581413944316562*I : 0.948655866506128 - 0.0381469928565034*I : 1.00000000000000) **********************************************************************
And the line above defining z implies random so...
comment:38 Changed 7 years ago by
On a different system (Debian amd64 experimental on an intel QuadCore2) I get
The following tests failed: sage -t -force_lib devel/sage/sage/modules/free_module_element.pyx # 2 doctests failed sage -t -force_lib devel/sage/sage/rings/real_mpfi.pyx # 5 doctests failed sage -t -force_lib devel/sage/sage/rings/real_mpfr.pyx # 12 doctests failed sage -t -force_lib devel/sage/sage/rings/complex_interval_field.py # 3 doctests failed sage -t -force_lib devel/sage/sage/rings/complex_field.py # 4 doctests failed sage -t -force_lib devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 2 doctests failed sage -t -force_lib devel/sage/sage/matrix/constructor.py # 2 doctests failed sage -t -force_lib devel/sage/sage/matrix/matrix2.pyx # 3 doctests failed sage -t -force_lib devel/sage/sage/matrix/matrix_mpolynomial_dense.pyx # 2 doctests failed sage -t -force_lib devel/sage/sage/misc/randstate.pyx # 13 doctests failed ----------------------------------------------------------------------
This also is on top of 4.8, but I could not launch sage before also rebuilding MPIR which was not necessary on the other system.
comment:39 in reply to: ↑ 28 Changed 7 years ago by
- Work issues changed from failing doctests for 32 bits to correct doctests
Replying to zimmerma:
I tried importing the MPFI spkg+patches (#12171) and that spkg on top of 4.8.alpha6 on a 32-bit Pentium 4 and got the following failures:
sage -t "devel/sage-main/sage/matrix/matrix_mpolynomial_dense.pyx" sage -t "devel/sage-main/sage/matrix/constructor.py" sage -t "devel/sage-main/sage/matrix/matrix2.pyx" sage -t "devel/sage-main/sage/misc/randstate.pyx" sage -t "devel/sage-main/sage/modules/free_module_element.pyx" sage -t "devel/sage-main/sage/rings/real_mpfi.pyx" sage -t "devel/sage-main/sage/rings/complex_field.py" sage -t "devel/sage-main/sage/rings/complex_interval_field.py" sage -t "devel/sage-main/sage/rings/real_mpfr.pyx" # Killed/crashed sage -t "devel/sage-main/sage/schemes/elliptic_curves/ell_rational_field.py"
Paul
My last failures happen in the same files as what you posted some time ago and are always involving some random function.
As you posted later, this should be because MPFR changed its behavior.
I'll post a patch fixing the doctest, but cannot test it on a 32 bits computer (nor exotic architectures), so I'll leave that to someone else.
The question is whether this change of behavior is actually a problem ?
If someone saved old stuff with a given random seed, or something like that, its results will not be reproducible... I do not really feel concerned about that.
If someone could also test the spkg on sunos, solaris, apple or other exotic oses/cpus combinations, that would be more than welcome.
comment:40 Changed 7 years ago by
After rebuilding mpir and atlas (which was screwed) on my core i7 setup, here is what I get
---------------------------------------------------------------------- The temporary doctesting directory /home/jp/.sage/tmp/jp_x220-9822 was not removed: it is not empty, presumably because doctests failed or doctesting was interrupted. ---------------------------------------------------------------------- The following tests failed: sage -t -force_lib devel/sage/sage/modular/modsym/space.py # 0 doctests failed sage -t -force_lib devel/sage/sage/modules/free_module_element.pyx # 2 doctests failed sage -t -force_lib devel/sage/sage/matrix/matrix_mpolynomial_dense.pyx # 2 doctests failed sage -t -force_lib devel/sage/sage/matrix/constructor.py # 2 doctests failed sage -t -force_lib devel/sage/sage/matrix/matrix2.pyx # 3 doctests failed sage -t -force_lib devel/sage/sage/rings/real_mpfi.pyx # 5 doctests failed sage -t -force_lib devel/sage/sage/rings/complex_field.py # 4 doctests failed sage -t -force_lib devel/sage/sage/rings/real_mpfr.pyx # 12 doctests failed sage -t -force_lib devel/sage/sage/rings/complex_interval_field.py # 3 doctests failed sage -t -force_lib devel/sage/sage/tests/cmdline.py # 4 doctests failed sage -t -force_lib devel/sage/sage/misc/randstate.pyx # 13 doctests failed sage -t -force_lib devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 2 doctests failed
I guess something went wrong with that installation because the failing doctest in cmdline.pyx really look strange.
comment:41 in reply to: ↑ 27 Changed 7 years ago by
Replying to leif:
If you have enough time (and/or the machine is fast enough), you can copy new/updated spkgs into
$SAGE_ROOT/spkg/standard/
and run$ env SAGE_UPGRADING=yes make buildafterwards. That way all dependent packages automatically get rebuilt, which can take some time.
should I rename new packages with the old name? Or keep the new name?
For example Sage 4.8 contains mpfi-1.3.4-cvs20071125.p9.spkg
. Should I put
mpfi-1.5.1.spkg
aside this one, or rename it?
Paul
comment:42 Changed 7 years ago by
I just ran the doctests on cicero (skynet) with vanilla 4.8 and all of them pass.
Paul
comment:43 Changed 7 years ago by
Just to confirm my above statements, on a third amd64 ubuntu system, I get the same errors as above, except the two "strange" ones, i.e. cmdline.py and space.py.
comment:44 Changed 7 years ago by
And those last tests were on top sage-5.0.beta4
comment:45 Changed 7 years ago by
I've had a look at the random MPFR real number generation code and there was indeed a "hack" for 32 bits systems before which requested more bits (31 from randstate doc, although real_mpfr says 32) from MPIR to get an equivalent MPFR behavior. Although, if I force sage to do the same on my 64 bits system (just adding a call to rstate.c_random() unconditionally in real_mpfr), I do not get what 32 bits (and 64 bits) systems used to return (nor what my 64 bits system now return which was to expect).
Paul, I'm confused by your last message. By vanilla 4.8, do you mean vanilla+this ticket and dependencies? If that's the case, and I somehow understand correctly the random generation stuff of MPIR and MPFR which should both be hardware independent now, the change I made above should give me a similar behavior as you on a 32 bits system and get no doctest failures.
For example the last failing test in real_mpfr used to give
0.979095507956490
With this ticket and dependencies, I get (on all of my 64 systems)
-0.616678906367394
With the additional call to c_random() I get (on all of my 64 systems)
0.681934947736557
comment:46 follow-up: ↓ 47 Changed 7 years ago by
Paul, I'm confused by your last message. By vanilla 4.8, do you mean vanilla+this ticket and dependencies?
no, I just meant vanilla 4.8. This was just to make sure that potential failing doctests did not fail before this patch.
Now with #12171 all doctests still pass (with one timeout even with -long
,
for a test taking more than half an hour).
I'll now try with this spkg.
Paul
comment:47 in reply to: ↑ 46 Changed 7 years ago by
Replying to zimmerma:
no, I just meant vanilla 4.8. This was just to make sure that potential failing doctests did not fail before this patch.
That's good news for the tests I ran above. I guess the piece of hack in real_mpfr should now be removed. Could you try running the tests with and without the c_random() stuff in real_mpfr and post the values you get for the last failing (I guess it will fail as well) in real_mpfr (on 32 bits I mean)?
The bad news is that we won't be able (or it will be very hackish???) to reproduce the previous behavior of Sage, thus breaking all use code depending on the deterministic behavior of PRNG.
comment:48 Changed 7 years ago by
Could you try running the tests with and without the c_random() stuff in real_mpfr [...]
ok, I'll first run the tests with the attached spkg. Currently running
env SAGE_UPGRADING=yes make build
, this will take some time since cicero is slow.
Paul
comment:49 Changed 7 years ago by
Jean-Pierre,
I've had a look at the random MPFR real number generation code and there was indeed a "hack" for 32 bits systems before which requested more bits (31 from randstate doc, although real_mpfr says 32) from MPIR to get an equivalent MPFR behavior. Although, if I force sage to do the same on my 64 bits system (just adding a call to rstate.c_random() unconditionally in real_mpfr), I do not get what 32 bits (and 64 bits) systems used to return (nor what my 64 bits system now return which was to expect).
this is expected. If you add the call to rstate.c_random()
unconditionally,
then on 64-bit systems you will draw 64 extra random bits when the precision mod 64 is between 1 and 32, whereas on 32-bit systems you will draw only 32 extra random bits.
In fact, since version 3.1.0, MPFR leaves the GMP random generator in the same state, whatever the number of bits per limb. See changeset 7133 from MPFR, and http://gmplib.org/list-archives/gmp-devel/2010-September/001642.html. Thus the fix is simply to remove the "gross hack" at lines 930-942 in real_mpfr.pyx.
Paul
comment:50 Changed 7 years ago by
From what i saw in randstate, c_random calls gmp_urandomb_ui(self.gmp_state, 31).
So this has different behaviors on the gmp rand internal state on 32 and 64 bits ?
I expected that it didn't, so that adding a call to c_random() even on 64 bits (just to see, I don't plan on integrating it to Sage, removing the hack is indeed the right solution) would produce identical behaviors on 32 and 64 bits, whence my request for you to test it (in fact with the hack and without it, not with a call to c_random added, on 32 bits).
I also had little hope that it could revert the real PNRG to its previous behavior for a given seed (in order not to break existing code, outside Sage source tree I mean).
Anyway, if the mpfr code changed, its completely understandable that the random output changes, even with the same seed, so this is hopeless and not really harmful (to me at least).
Anyway, if you confirm that you get the same values that I get (with no call to c_random at all, ie without the hackish section only running on 32 bits currently), I'll post patches to remove the hack section and correct the doctests.
comment:51 Changed 7 years ago by
From what i saw in randstate, c_random calls gmp_urandomb_ui(self.gmp_state, 31). So this has different behaviors on the gmp rand internal state on 32 and 64 bits ?
no it shouldn't. But beware that Sage uses MPIR, not GMP. We should check that MPIR gives the same behaviour for the random state on 32-bit and 64-bit processors.
I'll tell you what I get as soon as make build
finished...
Paul
comment:52 Changed 7 years ago by
The files randbui.c where gmp_urandomb_ui is defined are similar in GMP (5.04) and MPIR (2.1.3.p9 in Sage).
They only call _gmp_rand defined in gmp-impl.h which looks quite similar as well (in fact I saw no diff but did not check completely, there could be some black magic somewhere deep enough).
From what I see in integer_ring.pyx and more specifically in the _randomize_mpz function where c_random is called as well, there is no different sections for 32 and 64 bits, nor different doctest in randstate.pyx (only rgp from Pari does change), so I guess it is not that unsafe to assume that MPIR behaves the same for both.
comment:53 Changed 7 years ago by
Jean-Pierre,
the test of real_mpfr
failed with a Segmentation fault on cicero. However I get
the following with the "gross hack" on cicero (32-bit Pentium 4):
---------------------------------------------------------------------- | Sage Version 4.8, Release Date: 2012-01-20 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- sage: R=RealField(65) sage: set_random_seed(42) sage: R.random_element() 0.9396630990748764761 sage: R.random_element() 0.2987261287446042432
and without the "gross hack":
---------------------------------------------------------------------- | Sage Version 4.8, Release Date: 2012-01-20 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- sage: R=RealField(65) sage: set_random_seed(42) sage: R.random_element() 0.7846023201112678858 sage: R.random_element() 0.3289891249716495404
You should be able to reproduce the later on a 64-bit machine.
Paul
comment:54 Changed 7 years ago by
Indeed.
When forcing the call to c_random(), I get
---------------------------------------------------------------------- | Sage Version 4.8, Release Date: 2012-01-20 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- Loading Sage library. Current Mercurial branch is: mpfr sage: R=RealField(65) sage: set_random_seed(42) sage: R.random_element() 0.9396630990748764761 sage: R.random_element() 0.2987261287446042432
And with "normal" behavior:
---------------------------------------------------------------------- | Sage Version 4.8, Release Date: 2012-01-20 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- Loading Sage library. Current Mercurial branch is: mpfr sage: R=RealField(65) sage: set_random_seed(42) sage: R.random_element() 0.7846023201112678858 sage: R.random_element() 0.3289891249716495404
So everything looks fine, except for the segfault you got.
Hopefully, I'll provide patches for removing the hackish section and fixing the doctests tomorrow.
comment:55 Changed 7 years ago by
- Keywords sd36 added
We want to work on the FLINT update at Sage Days 36 so I'll give this a try. I have access to a i7-920 running Linux i386 so it would be easy to test.
comment:56 Changed 7 years ago by
The segfault is from
sage: R = RealField(2147483647)
because MPFR contains code like
_srcs = (_srcprec + GMP_NUMB_BITS-1)/GMP_NUMB_BITS; _dests = (_destprec + GMP_NUMB_BITS-1)/GMP_NUMB_BITS - _srcs;
This gives an int overflow when prec=INT_MAX
, and soon after as segfault because the buffer sizes are wrong.
comment:57 Changed 7 years ago by
- Description modified (diff)
I think the easiest solution is to limit the maximal precision in Sage to be slightly below 32-bit INT_MAX. The attached patches do that.
comment:58 Changed 7 years ago by
- Report Upstream changed from N/A to Reported upstream. Little or no feedback.
- Status changed from needs_info to needs_review
comment:59 Changed 7 years ago by
- Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.
- Work issues correct doctests deleted
I ran the doctests on 32-bit and 64-bit machines and they work.
comment:60 Changed 7 years ago by
- Reviewers changed from Paul Zimmermann to Paul Zimmermann, Jean-Pierre Flori
- Status changed from needs_review to needs_work
- Work issues set to rebase on #12366
As I commented above, the current package needs to be rebased on #12366.
I'm doing it right now. I'll also fix the MPFI package so everything can finally be merged.
I had a look at your patches and have nothing to say :) So once I upload the new spkg, I guess it will be positive_review.
comment:61 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues rebase on #12366 deleted
Updated spkg is available at http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.p0.spkg
comment:62 Changed 7 years ago by
- Description modified (diff)
The above link doesn't point to the right file when clicked...
http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.p0.spkg
This one should be ok.
comment:63 Changed 7 years ago by
All test passed on my amd64 system.
If someone could have a final look at the updated spkg, I guess this can be finally set as positive_review.
comment:64 Changed 7 years ago by
- Status changed from needs_review to positive_review
The Solaris patch is applied to the wrong directory, it is currenty doing cp ../patches/mpn_exp.c mpn_exp.c
but if you want to overwrite the MPFR source then you should do cp ../patches/mpn_exp.c src/mpn_exp.c
(there is a src/src
directory in MPFR-3.1.0, this was just src/
in the old MPFR). Can you fix this as well?
comment:65 Changed 7 years ago by
- Status changed from positive_review to needs_work
Too fast with positive review ;-)
comment:66 Changed 7 years ago by
I wonder why some random tests still differ between 32-bit and 64-bit computers. Is there any reason?
Paul
comment:67 Changed 7 years ago by
I tried the patches on top of Sage 4.8 (after #12171) but the 2nd and 3rd failed to apply:
sage: hg_sage.import_patch("trac_11666_reduce_precision_max.patch") cd "/home/zimmerma/sage-4.8/devel/sage" && sage --hg import "/home/zimmerma/sage-4.8/trac_11666_reduce_precision_max.patch" applying /home/zimmerma/sage-4.8/trac_11666_reduce_precision_max.patch patching file sage/rings/real_mpfr.pyx Hunk #1 FAILED at 183 1 out of 1 hunks FAILED -- saving rejects to file sage/rings/real_mpfr.pyx.rej abort: patch failed to apply
and
sage: hg_sage.import_patch("trac_11666_fix_random_doctests.patch") cd "/home/zimmerma/sage-4.8/devel/sage" && sage --hg import "/home/zimmerma/sage-4.8/trac_11666_fix_random_doctests.patch" applying /home/zimmerma/sage-4.8/trac_11666_fix_random_doctests.patch patching file sage/misc/randstate.pyx Hunk #1 FAILED at 54 Hunk #2 FAILED at 85 Hunk #3 FAILED at 221 Hunk #4 FAILED at 233 Hunk #5 FAILED at 254 Hunk #6 FAILED at 276 6 out of 6 hunks FAILED -- saving rejects to file sage/misc/randstate.pyx.rej abort: patch failed to apply
Paul
comment:68 Changed 7 years ago by
- Status changed from needs_work to needs_review
Replying to zimmerma:
I wonder why some random tests still differ between 32-bit and 64-bit computers. Is there any reason? Paul
You mean in randstate.pyx? I guess that Pari RNG does not behaves the same on 32 and 64 bits. The other tests (MPIR, MPFR, GAP, NTL, Python) looks the same to me, but i might have missed something.
The spkg at the usual address is updated, if someone could test that the patch now applies on Solaris :)
For Paul: the Sage patches of the ticker did apply for me on top of 5.0.beta4
comment:69 Changed 7 years ago by
we decided in the MPFR development version to decrease the maximal value of the precision from 256, i.e., it is now 2^{31}-257. You might want to do the same in Sage, so that upgrading to future MPFR versions will not cause any problem.
Paul
Changed 7 years ago by
Updated patch to limit precision at 231-257 Updated patch to limit precision at 231-257 Updated patch to limit precision at 231-257
comment:70 Changed 7 years ago by
- Reviewers changed from Paul Zimmermann, Jean-Pierre Flori to Paul Zimmermann, Jean-Pierre Flori, Volker Braun
- Status changed from needs_review to positive_review
Looks good. I'm happy to give the spkg a positive review. Nothing changed in mpn_exp.c
except the file location, so I don't foresee any Solaris issues.
I've implemented Paul's suggestion about the precision limit, which was the only reviewer complaint. So I'll take it that everyone agrees with positive review ;-)
Patches apply cleanly against sage-5.0.alpha4 and alpha5.
comment:71 Changed 7 years ago by
- Dependencies changed from #12171 to #12171, #12548
- Status changed from positive_review to needs_work
Sorry for the trouble, but this should be rebased to #12548.
comment:72 Changed 7 years ago by
- Status changed from needs_work to needs_review
Here you go.
Same address as usual.
comment:73 Changed 7 years ago by
- Status changed from needs_review to needs_work
SPKG.txt
refers incorrectly to mpfr-3.1.0.p1
, it should be .p0
comment:75 Changed 7 years ago by
- Reviewers changed from Paul Zimmermann, Jean-Pierre Flori, Volker Braun to Paul Zimmermann, Jean-Pierre Flori, Volker Braun, Jeroen Demeyer
- Status changed from needs_review to positive_review
Looks good!
comment:76 Changed 7 years ago by
I'm not sure my advice in comment 11 to configure MPFR with -disable-thread-safe
was taken into account. This might cause problems on various systems, see for example
http://www.loria.fr/~zimmerma/software/compilerbugs.html
In addition, I'm not sure MPFR can be used as multi-thread within Sage.
Paul
comment:77 Changed 7 years ago by
No, I did not. I just started on top of Mikes package which came before your comment and I don't remmeber that MPFR includes that flag by default. Ill send a new package tomorrow if you think that's needed. Too late for today.
comment:78 Changed 7 years ago by
- Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.
- Status changed from positive_review to needs_work
I'm pretty sure we don't use multithreading with MPFR so we can just pass --disable-thread-safe
to configure. I'll be happy to review it tomorrow ;-)
comment:79 Changed 7 years ago by
Jean-Pierre, the -disable-thread-safe
is not needed, but since Sage does not use
multithreading with MPFR, and given the poor support of multithreading by some compilers and operating systems, I recommend disabling thread local storage.
Paul
comment:80 Changed 7 years ago by
- Status changed from needs_work to needs_review
SPKG updated at the same address.
comment:81 Changed 7 years ago by
Is there anything preventing this from getting positive review ?
The package address did not change, so I've not updated the ticket description.
comment:82 Changed 7 years ago by
Here is a review of trac_11666_spkg.patch
:
there are small typos in SPKG.txt
, which were already there before:
coment
should be comment
,
occured
should be occurred
,
reccommened
should be recommended
.
Apart from that, the more important would be to test the modified spkg.
If this was already tested by the buildbot, then I will give a positive review.
Paul Zimmermann
comment:83 follow-up: ↓ 85 Changed 7 years ago by
- Status changed from needs_review to needs_work
- Work issues set to typos
From http://wiki.sagemath.org/buildbot it seems that the patchbot does not feel concerned by automatically testing spkg and I have no idea how to use Sage facilities to perform automatically such tests. I even think I have no account on the needed machines, so I cannot take care of launching automated tests.
Anyway, I'll take the typos into account tomorrow and update the spkg.
comment:84 Changed 7 years ago by
- Status changed from needs_work to needs_review
In fact I had some time right now and corrected the typos.
The package is at the same address as before.
comment:85 in reply to: ↑ 83 Changed 7 years ago by
Replying to jpflori:
From http://wiki.sagemath.org/buildbot it seems that the patchbot does not feel concerned by automatically testing spkg and I have no idea how to use Sage facilities to perform automatically such tests. I even think I have no account on the needed machines, so I cannot take care of launching automated tests.
Yes, there is currently no way to automatically test spkgs.
comment:86 follow-ups: ↓ 87 ↓ 88 Changed 7 years ago by
- Status changed from needs_review to positive_review
The current workflow is positive review -> release manage tests beta on build bot. While the authors should of course test patches, you don't have to test it on every supported machine to give a positive review.
We did test it on 32 and 6-bit machines which is the most likely source of errors. Specifics for other systems were not changed so its likely that it'll continue to work. I'll give the spell-checked SPKG.txt my positive review.
comment:87 in reply to: ↑ 86 Changed 7 years ago by
Replying to vbraun:
We did test it on 32 and 6-bit machines which is the most likely source of errors.
Agreed. My impression from Paul Zimmermann's comment was that it hadn't been tested at all.
comment:88 in reply to: ↑ 86 Changed 7 years ago by
comment:89 Changed 7 years ago by
- Work issues typos deleted
comment:90 Changed 7 years ago by
- Merged in set to sage-5.0.beta7
- Resolution set to fixed
- Status changed from positive_review to closed
The second release candidate is out:
--enable-gmp-internals
.mpfr_cmp_q()
andmpfr_cmp_f()
(fixing the problem with theerange
flag in particular).mpfr_buildopt_gmpinternals_p()
function.Final release now scheduled for October 3rd.