search

sagemath / sage

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

Make arb a standard package #18546

Closed mezzarobba closed 9 years ago

mezzarobba commented 9 years ago

Arb has been an optional package for a few months. This is a proposal to fast-track it to standard, as suggested in <c02d759d-9a81-4c85-a110-a2e2ffd679dc@googlegroups.com>.

See also: #17218, #17194, #16747

Depends on #19280 Depends on #19443

CC: @cheuberg @fredrik-johansson @simon-king-jena

Component: packages: standard

Author: Marc Mezzarobba, Clemens Heuberger

Branch/Commit: ea5741f

Reviewer: Clemens Heuberger, Marc Mezzarobba

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

mezzarobba commented 9 years ago

Author: Marc Mezzarobba

mezzarobba commented 9 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
+[Arb](http://fredrikj.net/arb/) has been an optional package for a few months. This is a proposal to fast-track it to standard, as suggested in `<c02d759d-9a81-4c85-a110-a2e2ffd679dc@googlegroups.com>`.

+See also: #17218, #17194, #16747
mezzarobba commented 9 years ago

Commit: 85dfca5

mezzarobba commented 9 years ago

Branch: u/mmezzarobba/18546-arb-std

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0b4b1f8#18546 Make arb a standard package
256547e#18546 Remove 'optional - arb' doctest flags
4c0e950#18546 Include modules depending on arb in the reference manual
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 85dfca5 to 4c0e950

mezzarobba commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 [Arb](http://fredrikj.net/arb/) has been an optional package for a few months. This is a proposal to fast-track it to standard, as suggested in `<c02d759d-9a81-4c85-a110-a2e2ffd679dc@googlegroups.com>`.

-See also: #17218, #17194, #16747
+See also: #16747, #17194, #17218, #17220, #17786
jhpalmieri commented 9 years ago
comment:4

It doesn't build for me on OS X 10.10:

    CC   ../build/partitions/partitions_fmpz_fmpz.o
    CC   ../build/partitions/partitions_hrr_sum_arb.o
    CC   ../build/partitions/partitions_rademacher_bound.o
    AR   libarb.a
ar: no archive members specified
usage:  ar -d [-TLsv] archive file ...
    ar -m [-TLsv] archive file ...
    ar -m [-abiTLsv] position archive file ...
    ar -p [-TLsv] archive [file ...]
    ar -q [-cTLsv] archive file ...
    ar -r [-cuTLsv] archive file ...
    ar -r [-abciuTLsv] position archive file ...
    ar -t [-TLsv] archive [file ...]
    ar -x [-ouTLsv] archive [file ...]
make[4]: *** [libarb.a] Error 1
make[3]: *** [library] Error 2
Error building arb.
jhpalmieri commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 [Arb](http://fredrikj.net/arb/) has been an optional package for a few months. This is a proposal to fast-track it to standard, as suggested in `<c02d759d-9a81-4c85-a110-a2e2ffd679dc@googlegroups.com>`.

-See also: #16747, #17194, #17218, #17220, #17786
+See also: #17218, #17194, #16747
mezzarobba commented 9 years ago
comment:7

Replying to @jhpalmieri:

It doesn't build for me on OS X 10.10:

I made this a separate issue (#18552).

mezzarobba commented 9 years ago

Dependencies: #18552

mezzarobba commented 9 years ago
comment:8

Replying to @jhpalmieri:

It doesn't build for me on OS X 10.10:

Could you please try with arb 2.6 (#18560)?

cheuberg commented 9 years ago

Changed dependencies from #18552 to #18552, #18560

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

adad105Trac 18560: Upgrade arb to 2.6.0
8d0b599Trac #18560: fix doctests after arb upgrade
922b674Trac #18560: spkg-{check,install} executable
0d9f8c7#18546 Make arb a standard package
016fe35#18546 Remove 'optional - arb' doctest flags
e97ac71#18546 Include modules depending on arb in the reference manual
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 4c0e950 to e97ac71

fchapoton commented 9 years ago
comment:11

does not apply, needs rebase

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fd8137d#18546 Make arb a standard package
58e1844#18546 Remove 'optional - arb' doctest flags
0400d43#18546 Include modules depending on arb in the reference manual
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from e97ac71 to 0400d43

cheuberg commented 9 years ago

Changed branch from u/mmezzarobba/18546-arb-std to u/cheuberg/18546-arb-std

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

91c5184Trac #18546: remove optional - arb in real_mpfi
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 0400d43 to 91c5184

cheuberg commented 9 years ago
comment:16

I reviewed this ticket. Including it into a fresh copy of develop (=6.9.beta4) builds fine with all doctests passing (three unrelated to this ticket only when called separately). In particular, the sage version built includes arb. Source code (removal of optional - arb) seems to be fine. Documentation seems to be ok.

There was one occurrence of optional arb in real_mpfi.pyx. I removed that as a reviewer patch. Please cross-review this change; then feel free to set to positive_review.

cheuberg commented 9 years ago

Reviewer: Clemens Heuberger

mezzarobba commented 9 years ago
comment:17

Thanks!

vbraun commented 9 years ago
comment:18

You can only depend on tickets that contribute code

vbraun commented 9 years ago

Changed dependencies from #18552, #18560 to #18560

vbraun commented 9 years ago
comment:19

Testsuite fails on 32-bit Linux

atan....PASS
atan2....FAIL: containment

a = (-143601044557104111888298409983 * 2^-97) +/- (0)

b = (-1 * 2^5) +/- (1040502623 * 2^-94)

c = (-1933204245030245427143485 * 2^-80) +/- (959240107 * 2^-104)

make[4]: *** [../build/arb/test/t-atan2_RUN] Aborted (core dumped)
atan_arf....g++ -pthread -shared -L/home/buildslave-sage/slave/sage_git/build/local/lib build/temp.linux-i686-2.7/home/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/matrix/matrix_cyclo_dense.o -L/home/buildslave-sage/slave/sage_git/build/local/lib -L/home/buildslave-sage/slave/sage_git/build/local/lib -lntl -lgmp -lstdc++ -lpython2.7 -o build/lib.linux-i686-2.7/sage/matrix/matrix_cyclo_dense.so
[ 75/170] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/buildslave-sage/slave/sage_git/build/local/include -I/home/buildslave-sage/slave/sage_git/build/local/include/python2.7 -I/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/numpy/core/include -I/home/buildslave-sage/slave/sage_git/build/src -I/home/buildslave-sage/slave/sage_git/build/src/sage/ext -I/home/buildslave-sage/slave/sage_git/build/src/build/cythonized -I/home/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/ext -I/home/buildslave-sage/slave/sage_git/build/local/include/python2.7 -c /home/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/matrix/matrix_modn_sparse.c -o build/temp.linux-i686-2.7/home/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/matrix/matrix_modn_sparse.o -fno-strict-aliasing -w -fno-tree-dominator-opts
FAIL: containment

a = (-9905938165778121085362896895 * 2^-91) +/- (0)

b = (-2031897670652875033840701129116896875534188457349248225 * 2^-180) +/- (538444801 * 2^-210)

make[4]: *** [../build/arb/test/t-atan_RUN] Aborted (core dumped)
vbraun commented 9 years ago
comment:20

http://build.sagedev.org/release/builders/%20%20fast%20Oxford%20arando%20%28Ubuntu%2013.04%20i686%29%20incremental/builds/538/steps/compile/logs/stdio

fredrik-johansson commented 9 years ago
comment:21

I can reproduce the failure on linux32 under virtualbox.

The source appears to be a bug in MPIR.

If you run

include "mpir.h"

int main() { mpz_t one, x, w; mpz_init(one); mpz_init(x); mpz_init(w); mpz_set_str(one, "62165404551223330269422781018352605012557018849668464680057997111644937126566671941632", 10); mpz_set_str(x, "39623752663112484341451587580", 10);

mpz_tdiv_q(w, one, x);
gmp_printf("%Zd / %Zd = %Zd\n", one, x, w);

}

it outputs 

62165404551223330269422781018352605012557018849668464680057997111644937126566671941632 / 39623752663112484341451587580 = 1568892406419848332919986945754866320479099155002496784035

but you can check with Python ints that the correct quotient should be

    1568892403497558507879962225846103176600476845510570267609
vbraun commented 9 years ago
comment:22

Some more failures on 32-bit:

sage -t --long src/sage/rings/complex_ball_acb.pyx
**********************************************************************
File "src/sage/rings/complex_ball_acb.pyx", line 1137, in sage.rings.complex_ball_acb.ComplexBall.accuracy
Failed example:
    CBF(I/2).accuracy()
Expected:
    9223372036854775807
Got:
    2147483647
**********************************************************************
File "src/sage/rings/complex_ball_acb.pyx", line 1139, in sage.rings.complex_ball_acb.ComplexBall.accuracy
Failed example:
    CBF('nan', 'inf').accuracy()
Expected:
    -9223372036854775807
Got:
    -2147483647
**********************************************************************
1 item had failures:
   2 of   5 in sage.rings.complex_ball_acb.ComplexBall.accuracy
    [294 tests, 2 failures, 0.32 s]
sage -t --long src/sage/rings/real_arb.pyx
**********************************************************************
File "src/sage/rings/real_arb.pyx", line 1430, in sage.rings.real_arb.RealBall.accuracy
Failed example:
    RBF(1).accuracy()
Expected:
    9223372036854775807
Got:
    2147483647
**********************************************************************
File "src/sage/rings/real_arb.pyx", line 1432, in sage.rings.real_arb.RealBall.accuracy
Failed example:
    RBF(NaN).accuracy()
Expected:
    -9223372036854775807
Got:
    -2147483647
**********************************************************************
1 item had failures:
   2 of   5 in sage.rings.real_arb.RealBall.accuracy
    [498 tests, 2 failures, 0.31 s]
wbhart commented 9 years ago
comment:23

Regarding the divion bug, can someone tell me the value of SB_DIVAPPR_Q_SMALL_THRESHOLD and SB_DIV_QR_SMALL_THRESHOLD on this machine, in gmp-mparam.h in MPIR.

If they are not 0, you will probably find setting them to 0 is a temporary workaround. If they are already 0 I wonder what on earth can have gone wrong.

I'm afraid I personally won't be able to fix this bug until late this month, but I have posted it on mpir-devel in case someone else can track it down.

If the gcc is 5.2 or above, it might also be worth building MPIR without optimisation (and check the CFLAGS doesn't contain -ansi : I don't think it does). We've had reports of what we think is miscompilation with recent gcc on Linux 32. We are still trying to track it down (code works fine under valgrind but otherwise does not).

fredrik-johansson commented 9 years ago
comment:24

Defining both SB_DIVAPPR_Q_SMALL_THRESHOLD and SB_DIV_QR_SMALL_THRESHOLD as 0 in gmp-mparam.h fixes the problem (these values are currently not defined at all).

The bug might be restricted to 32-bit installations on a 64-bit host, where MPIR has outdated tuneup values (my arch is x86/nehalem).

Since this is a rather serious bug affecting all of Sage, should it be a separate, blocker, ticket?

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:25

Replying to @fredrik-johansson:

Defining both SB_DIVAPPR_Q_SMALL_THRESHOLD and SB_DIV_QR_SMALL_THRESHOLD as 0 in gmp-mparam.h fixes the problem (these values are currently not defined at all).

The bug might be restricted to 32-bit installations on a 64-bit host, where MPIR has outdated tuneup values (my arch is x86/nehalem).

Since this is a rather serious bug affecting all of Sage, should it be a separate, blocker, ticket?

I'd say yes.

FWIW, I only tested the MPIR alphas stand-alone on Linux x86, on a true 32-bit processor.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

dbae43dTrac #18546: make doctests 32-bit aware
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 91c5184 to dbae43d

cheuberg commented 9 years ago
comment:27

Replying to @vbraun:

Some more failures on 32-bit:

sage -t --long src/sage/rings/complex_ball_acb.pyx
**********************************************************************
1 item had failures:
   2 of   5 in sage.rings.complex_ball_acb.ComplexBall.accuracy
    [294 tests, 2 failures, 0.32 s]
sage -t --long src/sage/rings/real_arb.pyx

These two failures do not belong to this ticket, but to #19063 (the relevant code is not present here).

File "src/sage/rings/real_arb.pyx", line 1430, in sage.rings.real_arb.RealBall.accuracy
1 item had failures:
   2 of   5 in sage.rings.real_arb.RealBall.accuracy
    [498 tests, 2 failures, 0.31 s]

I pushed a fix for this, but have not yet tested it under 32 bit.

cheuberg commented 9 years ago
comment:28

Replying to @fredrik-johansson:

Since this is a rather serious bug affecting all of Sage, should it be a separate, blocker, ticket?

I opened #19280 for that.

cheuberg commented 9 years ago
comment:29

Arb's testsuite passes on 32 bit once mpir has been rebuilt using #19280.

cheuberg commented 9 years ago

Changed dependencies from #18560 to #18560, #19280

cheuberg commented 9 years ago
comment:30

Replying to @cheuberg:

I pushed a fix for this, but have not yet tested it under 32 bit.

Now, after recompiling mpir with #19280 applied, all tests pass on my (virtual) 32 bit machine.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from dbae43d to 4e1af06

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

f1b4dffTrac #19280: add test to mpir's test suite
b5049f6Trac #19280: add fix to all x86 gmp-mparam
be00675Trac #19280: increase patch level
0d48fb5Trac #19280: Add header to patch files
4e1af06Trac #18546: Merge #19280 to fix mpir problem on 32 bit
mezzarobba commented 9 years ago
comment:32

Thanks for taking care of this!

mezzarobba commented 9 years ago

Changed reviewer from Clemens Heuberger to Clemens Heuberger, Marc Mezzarobba

mezzarobba commented 9 years ago

Changed author from Marc Mezzarobba to Marc Mezzarobba, Clemens Heuberger

jdemeyer commented 9 years ago
comment:33

Needs to be rebased to the latest version of #19280

jdemeyer commented 9 years ago

Changed dependencies from #18560, #19280 to #19280

mezzarobba commented 9 years ago
comment:34

Replying to @jdemeyer:

Needs to be rebased to the latest version of #19280

Are you saying we should rebase these changes (and the other branches that depend on them) on top of #19820? I can do it, but in this case I think we should also rebase #19280 itself on top of sage-6.9.

Or do you want me to merge the branch there into this branch? (I'll do it if you want, but why? The other ticket is already a dependency, so this ticket won't get merged without it...)

jdemeyer commented 9 years ago
comment:35

Replying to @mezzarobba:

Replying to @jdemeyer:

Needs to be rebased to the latest version of #19280

Are you saying we should rebase these changes (and the other branches that depend on them) on top of #19820? I can do it, but in this case I think we should also rebase #19280 itself on top of sage-6.9.

Or do you want me to merge the branch there into this branch? (I'll do it if you want, but why? The other ticket is already a dependency, so this ticket won't get merged without it...)

Rebase or merge: either is fine for me. The reason I ask it is for easier review/testing. It's better if I can just do git trac checkout <ticket number> instead of manually needing to merge the dependencies. The fact that this branch has a wrong version of #19280 makes it even worse.

mezzarobba commented 9 years ago

New commits:

0f4e373Revert "Trac #19280: add fix to all x86 gmp-mparam"
f780291Trac #19280: Use patch proposed in [#19280 comment:5](https://github.com/sagemath/sage/issues/19280#comment:5)
8cf7d18Merge trac/u/cheuberg/mpir-bug into 6.9
ef257db#18546 Make arb a standard package
a7a41bc#18546 Remove 'optional - arb' doctest flags
0e2e8ea#18546 Include modules depending on arb in the reference manual
76883adTrac #18546: remove optional - arb in real_mpfi
4dbae98Trac #18546: make doctests 32-bit aware