sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 481 forks source link

Upgrade MPFR to 3.1.0 #11666

Closed 83660e46-0051-498b-a8c1-f7a7bd232b5a closed 12 years ago

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

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:

Depends on #12171 Depends on #12548

Upstream: Fixed upstream, but not in a stable release.

CC: @jpflori @zimmermann6

Component: packages: standard

Keywords: sd36 sd32 MPFR spkg wishlist sd35.5

Author: Mike Hansen, Jean-Pierre Flori, Volker Braun

Reviewer: Paul Zimmermann, Jean-Pierre Flori, Volker Braun, Jeroen Demeyer

Merged: sage-5.0.beta7

Issue created by migration from https://trac.sagemath.org/ticket/11666

jpflori commented 12 years ago
comment:43

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.

jpflori commented 12 years ago
comment:44

And those last tests were on top sage-5.0.beta4

jpflori commented 12 years ago
comment:45

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
zimmermann6 commented 12 years ago
comment:46

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

jpflori commented 12 years ago
comment:47

Replying to @zimmermann6:

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.

zimmermann6 commented 12 years ago
comment:48

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

zimmermann6 commented 12 years ago
comment:49

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

jpflori commented 12 years ago
comment:50

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.

zimmermann6 commented 12 years ago
comment:51

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

jpflori commented 12 years ago
comment:52

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.

zimmermann6 commented 12 years ago
comment:53

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

jpflori commented 12 years ago
comment:54

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.

vbraun commented 12 years ago

Changed keywords from sd32 MPFR spkg wishlist sd35.5 to sd36 sd32 MPFR spkg wishlist sd35.5

vbraun commented 12 years ago
comment:55

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.

vbraun commented 12 years ago
comment:56

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.

vbraun commented 12 years ago

Initial patch

vbraun commented 12 years ago

Attachment: trac_11666_remove_random_hack.patch.gz

Initial patch

vbraun commented 12 years ago
comment:57

Attachment: trac_11666_fix_random_doctests.patch.gz

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.

vbraun commented 12 years ago

Description changed:

--- 
+++ 
@@ -2,29 +2,8 @@

 Use http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.spkg

----
-**MPFR 3.0.1** was released on April 4th 2011; there are already some [additional upstream patches](http://www.mpfr.org/mpfr-current/allpatches) for that version; see [http://www.mpfr.org/mpfr-current/](http://www.mpfr.org/mpfr-current/).
+Apply:
+* [attachment: trac_11666_remove_random_hack.patch](https://github.com/sagemath/sage-prod/files/10653426/trac_11666_remove_random_hack.patch.gz)
+* [attachment: trac_11666_reduce_precision_max.patch](https://github.com/sagemath/sage-prod/files/10653428/trac_11666_reduce_precision_max.patch.gz)
+* [attachment: trac_11666_fix_random_doctests.patch](https://github.com/sagemath/sage-prod/files/10653427/trac_11666_fix_random_doctests.patch.gz)

----
-**MPFR 3.1.0** [has been released October 3rd 2011](http://www.mpfr.org/mpfr-3.1.0/).
-
-## Changes from versions 3.0.* to version 3.1.0:
-* The MPFR source has been reorganized.
-* Dropped `ansi2knr` support.
-* TLS support is now detected automatically. If TLS is supported, MPFR is built as thread safe by default. To disable TLS explicitly, configure MPFR with `--disable-thread-safe`.
-* New `--enable-gmp-internals` `configure` option to use GMP's undocumented functions (not from the public API). Note that library versioning is not guaranteed to work if this option is used.
-* The `mpfr_urandom()` and `mpfr_urandomb()` functions now return identical values on processors with different word size (assuming the same random seed, and since the GMP random generator does not depend itself on the word size, cf. [http://gmplib.org/list-archives/gmp-devel/2010-September/001642.html](http://gmplib.org/list-archives/gmp-devel/2010-September/001642.html)).
-* The `mpfr_add_one_ulp()` and `mpfr_sub_one_ulp()` macros (which are obsolete and no more documented) will be removed in a future release.
-* Speed improvement for the `mpfr_sqr()` and `mpfr_div()` functions using Mulders' algorithm. As a consequence, other functions using those routines are also faster.
-* Much faster formatted output (`mpfr_printf()`, etc.) with `%Rg` and similar.
-* The `--with-gmp-build` `configure` option can now be used when the GMP source directory and the GMP build directory are different (without having to copy header files manually as before).
-* New functions `mpfr_buildopt_tune_case()`, `mpfr_frexp()`, `mpfr_grandom()` and `mpfr_z_sub()`.
-* New division-by-zero exception (flag) and associated functions.
-* The `mpfr.h` header can be included several times, while still supporting optional functions (see Section "Headers and Libraries" in the manual).
-* Updated tuning parameters.
-* Improved MPFR manual.
-* MPFR tests: `libtool` no longer generates wrapper scripts with `make check` (so that running the tests under `valgrind` or `gdb` is easier).
-* Bug fixes.
- Note: The `mpfr_subnormalize()` implementation up to MPFR 3.0.0 did not change the flags. In particular, it did not follow the generic rule concerning the inexact flag (and no special behavior was specified). The case of the underflow flag was more a lack of specification.
-
-Tarballs are available in various formats (regarding the compression method); a bzipped one can be found [here](http://www.mpfr.org/mpfr-3.1.0/mpfr-3.1.0.tar.bz2) (upstream).
vbraun commented 12 years ago
comment:58

Reported upstream at https://gforge.inria.fr/tracker/index.php?func=detail&aid=13918&group_id=136&atid=619

vbraun commented 12 years ago

Upstream: Reported upstream. Little or no feedback.

vbraun commented 12 years ago

Changed upstream from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.

vbraun commented 12 years ago

Changed author from Mike Hansen, Jean-Pierre Flori to Mike Hansen, Jean-Pierre Flori, Volker Braun

vbraun commented 12 years ago

Changed work issues from correct doctests to none

vbraun commented 12 years ago
comment:59

I ran the doctests on 32-bit and 64-bit machines and they work.

jpflori commented 12 years ago

Work Issues: rebase on #12366

jpflori commented 12 years ago

Changed reviewer from Paul Zimmermann to Paul Zimmermann, Jean-Pierre Flori

jpflori commented 12 years ago
comment:60

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.

jpflori commented 12 years ago
comment:61

Updated spkg is available at http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.p0.spkg

jpflori commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,9 +1,9 @@
 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](http://wiki.sagemath.org/days32/wishlist).

-Use http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.spkg
+Use [http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.p0.spkg](http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.spkg)

 Apply:
+
 * [attachment: trac_11666_remove_random_hack.patch](https://github.com/sagemath/sage-prod/files/10653426/trac_11666_remove_random_hack.patch.gz)
 * [attachment: trac_11666_reduce_precision_max.patch](https://github.com/sagemath/sage-prod/files/10653428/trac_11666_reduce_precision_max.patch.gz)
 * [attachment: trac_11666_fix_random_doctests.patch](https://github.com/sagemath/sage-prod/files/10653427/trac_11666_fix_random_doctests.patch.gz)
-
jpflori commented 12 years ago

Changed work issues from rebase on #12366 to none

jpflori commented 12 years ago
comment:62

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.

jpflori commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 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](http://wiki.sagemath.org/days32/wishlist).

-Use [http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.p0.spkg](http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.spkg)
+Use http://perso.telecom-paristech.fr/~flori/sage/mpfr-3.1.0.p0.spkg

 Apply:
jpflori commented 12 years ago
comment:63

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.

vbraun commented 12 years ago
comment:64

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?

vbraun commented 12 years ago
comment:65

Too fast with positive review ;-)

zimmermann6 commented 12 years ago
comment:66

I wonder why some random tests still differ between 32-bit and 64-bit computers. Is there any reason?

Paul

zimmermann6 commented 12 years ago
comment:67

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

jpflori commented 12 years ago
comment:68

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

zimmermann6 commented 12 years ago
comment:69

we decided in the MPFR development version to decrease the maximal value of the precision from 256, i.e., it is now 231-257. You might want to do the same in Sage, so that upgrading to future MPFR versions will not cause any problem.

Paul

vbraun commented 12 years ago

Updated patch to limit precision at 231-257 Updated patch to limit precision at 231-257 Updated patch to limit precision at 2**31-257

vbraun commented 12 years ago
comment:70

Attachment: trac_11666_reduce_precision_max.patch.gz

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.

vbraun commented 12 years ago

Changed reviewer from Paul Zimmermann, Jean-Pierre Flori to Paul Zimmermann, Jean-Pierre Flori, Volker Braun

jdemeyer commented 12 years ago

Changed dependencies from #12171 to #12171, #12548

jdemeyer commented 12 years ago
comment:71

Sorry for the trouble, but this should be rebased to #12548.

jpflori commented 12 years ago
comment:72

Here you go.

Same address as usual.

jdemeyer commented 12 years ago
comment:73

SPKG.txt refers incorrectly to mpfr-3.1.0.p1, it should be .p0

jpflori commented 12 years ago
comment:74

Corrected.

vbraun commented 12 years ago

Changed reviewer from Paul Zimmermann, Jean-Pierre Flori, Volker Braun to Paul Zimmermann, Jean-Pierre Flori, Volker Braun, Jeroen Demeyer

vbraun commented 12 years ago
comment:75

Looks good!