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 jpflori)

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)

trac_11666_remove_random_hack.patch (1.4 KB) - added by vbraun 7 years ago.
Initial patch
trac_11666_fix_random_doctests.patch (25.0 KB) - added by vbraun 7 years ago.
Initial patch
trac_11666_reduce_precision_max.patch (1.6 KB) - added by vbraun 7 years ago.
Updated patch to limit precision at 231-257 Updated patch to limit precision at 231-257 Updated patch to limit precision at 231-257
trac_11666_spkg.patch (24.5 KB) - added by jpflori 7 years ago.
spkg diff, for review only

Download all attachments as: .zip

Change History (94)

comment:1 Changed 8 years ago by was

  • Keywords sd32 added

comment:2 Changed 8 years ago by jpflori

  • Cc jpflori added

comment:3 Changed 8 years ago by leif

  • 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 8 years ago by leif

  • Description modified (diff)

The second release candidate is out:

[...] The main changes since this first release candidate are:

  • Fixed --enable-gmp-internals.
  • Handle the special cases in mpfr_cmp_q() and mpfr_cmp_f() (fixing the problem with the erange flag in particular).
  • Added mpfr_buildopt_gmpinternals_p() function.
  • MPFR manual update and minor corrections.

Here's a second release candidate. As there should not be new platform-specific problems, the final release is delayed by a few days only.

[...]
http://www.mpfr.org/mpfr-3.1.0/mpfr-3.1.0-rc2.tar.bz2
[...]

The MD5's:
[...]
6ba48c87687959d5e68cd695686257f4 mpfr-3.1.0-rc2.tar.bz2
[...]

[...]

Final release now scheduled for October 3rd.

comment:5 Changed 7 years ago by leif

  • 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 mhansen

  • Authors set to Mike Hansen
  • 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 mhansen

  • 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 fbissey

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 mhansen

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 zimmerma

  • Cc zimmerma added

comment:11 follow-up: Changed 7 years ago by 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.

Paul

comment:12 Changed 7 years ago by zimmerma

  • 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 fbissey

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 mhansen

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 fbissey

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 jpflori

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 zimmerma

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: Changed 7 years ago by zimmerma

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 fbissey

Replying to zimmerma:

I've tested this spkg on top of #12171 on my 64-bit laptop (Core 2 Duo): all doctests pass.

Yes 64bit is fine as far as I can tell. It's 32bit that's the trouble - both linux and OS X [10.5.8], don't know about other like solaris or cygwin.

comment:20 Changed 7 years ago by zimmerma

  • Keywords sd35.5 added

comment:21 follow-up: Changed 7 years ago by zimmerma

  • 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: Changed 7 years ago by 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
  • 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: Changed 7 years ago by jpflori

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 leif

comment:25 in reply to: ↑ 22 Changed 7 years ago by leif

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:27 in reply to: ↑ 21 ; follow-up: Changed 7 years ago by leif

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: Changed 7 years ago by zimmerma

  • 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 zimmerma

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 jpflori

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 jpflori

  • Authors changed from Mike Hansen to Mike Hansen, Jean-Pierre Flori
  • 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 jpflori

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 zimmerma

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 zimmerma

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 jpflori

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 jpflori

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 jpflori

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 jpflori

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 jpflori

  • 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 jpflori

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 zimmerma

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 build

afterwards. 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 zimmerma

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 jpflori

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 jpflori

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

comment:45 Changed 7 years ago by jpflori

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: Changed 7 years ago by zimmerma

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 jpflori

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 zimmerma

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 zimmerma

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 jpflori

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 zimmerma

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 jpflori

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 zimmerma

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 jpflori

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 vbraun

  • 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 vbraun

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.

Changed 7 years ago by vbraun

Initial patch

Changed 7 years ago by vbraun

Initial patch

comment:57 Changed 7 years ago by vbraun

  • 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 vbraun

  • 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 vbraun

  • Authors changed from Mike Hansen, Jean-Pierre Flori to Mike Hansen, Jean-Pierre Flori, Volker Braun
  • 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 jpflori

  • 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 jpflori

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues rebase on #12366 deleted

comment:62 Changed 7 years ago by jpflori

  • 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 jpflori

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 vbraun

  • 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 vbraun

  • Status changed from positive_review to needs_work

Too fast with positive review ;-)

comment:66 Changed 7 years ago by zimmerma

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 zimmerma

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 jpflori

  • 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 zimmerma

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

Changed 7 years ago by vbraun

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 vbraun

  • 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 jdemeyer

  • 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 jpflori

  • Status changed from needs_work to needs_review

Here you go.

Same address as usual.

comment:73 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:74 Changed 7 years ago by jpflori

  • Status changed from needs_work to needs_review

Corrected.

comment:75 Changed 7 years ago by vbraun

  • 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 zimmerma

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 jpflori

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 vbraun

  • 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 zimmerma

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 jpflori

  • Status changed from needs_work to needs_review

SPKG updated at the same address.

comment:81 Changed 7 years ago by jpflori

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 zimmerma

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: Changed 7 years ago by jpflori

  • 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 jpflori

  • 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.

Changed 7 years ago by jpflori

spkg diff, for review only

comment:85 in reply to: ↑ 83 Changed 7 years ago by jdemeyer

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: Changed 7 years ago by vbraun

  • 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 jdemeyer

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 jdemeyer

Replying to vbraun:

We did test it on [...] 6-bit machines

Impressive!

comment:89 Changed 7 years ago by jdemeyer

  • Work issues typos deleted

comment:90 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.0.beta7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.