Opened 8 years ago
Closed 8 years ago
#15738 closed enhancement (fixed)
upgrade eclib to version 20140128
Reported by: | cremona | Owned by: | cremona |
---|---|---|---|
Priority: | major | Milestone: | sage-6.2 |
Component: | packages: standard | Keywords: | eclib |
Cc: | Merged in: | ||
Authors: | John Cremona | Reviewers: | Jeroen Demeyer |
Report Upstream: | Fixed upstream, in a later stable release. | Work issues: | |
Branch: | u/cremona/ticket/15738 | Commit: | 220c50e635fc85fb24b85378c87b37fbf01567b4 |
Dependencies: | Stopgaps: |
Description (last modified by )
There have been a lot of changes to eclib since the version 2012-08-30 in Sage-6.0. A lot of these are minor improvements and small bugfixes not all relevant to Sage. There is a new optional dependency on Flint (requiring Flint >=2.3) and an optional dependency on boost to parallelise some code not currently wrapped in Sage (a contribution by Warwick MSc student Marcus Mo). For details see https://github.com/JohnCremona/eclib.
Very few changes will be needed to the spkg-install
script, and the ECLIB dependency in build/deps will have FLINT added as a new dependency. There may be some cosmetic adjustments to doctest outputs necessary.
Upstream: http://boxen.math.washington.edu/home/cremona/eclib-20140128.tar.bz2
Change History (33)
comment:1 Changed 8 years ago by
- Description modified (diff)
comment:2 Changed 8 years ago by
- Branch set to u/cremona/ticket/15738
- Commit set to 32af9323edfbe7f2e954c8fcf13867c4f8a2c153
- Description modified (diff)
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
- Owner changed from (none) to cremona
There is only one doctest change: eclib now handles precision in saturation better, so something which used to fail now passes.
comment:4 Changed 8 years ago by
- Description modified (diff)
comment:5 follow-up: ↓ 8 Changed 8 years ago by
- Status changed from needs_review to needs_work
You should add a == License ==
section to SPKG.txt
.
comment:6 Changed 8 years ago by
Builds fine with SAGE_CHECK=yes
.
comment:7 Changed 8 years ago by
- Commit changed from 32af9323edfbe7f2e954c8fcf13867c4f8a2c153 to 315ab741adad959c8f43795ff0db7f9341c82023
Branch pushed to git repo; I updated commit sha1. New commits:
315ab74 | Added License section to eclib's SPKG.txt
|
comment:8 in reply to: ↑ 5 Changed 8 years ago by
- Status changed from needs_work to needs_review
Replying to jdemeyer:
You should add a
== License ==
section toSPKG.txt
.
Done. Thanks for reviewing.
comment:9 Changed 8 years ago by
- Status changed from needs_review to needs_work
When building, I get a warning about --with-gmp
. Is that supposed to happen?
eclib-20140126 ==================================================== Setting up build directory for eclib-20140126 Finished set up **************************************************** Host system: Linux tamiyo 3.7.10-gentoo #4 SMP PREEMPT Thu Oct 31 21:23:56 CET 2013 x86_64 Intel(R) Core(TM) i7-2640M CPU @ 2.80GHz GenuineIntel GNU/Linux **************************************************** C compiler: gcc C compiler version: Using built-in specs. COLLECT_GCC=/usr/x86_64-pc-linux-gnu/gcc-bin/4.6.3/gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-pc-linux-gnu/4.6.3/lto-wrapper Target: x86_64-pc-linux-gnu Configured with: /var/tmp/portage/sys-devel/gcc-4.6.3/work/gcc-4.6.3/configure --prefix=/usr --bindir=/usr/x86_64-pc-linux-gnu/gcc-bin/4.6.3 --includedir=/usr/lib/gcc/x 86_64-pc-linux-gnu/4.6.3/include --datadir=/usr/share/gcc-data/x86_64-pc-linux-gnu/4.6.3 --mandir=/usr/share/gcc-data/x86_64-pc-linux-gnu/4.6.3/man --infodir=/usr/share /gcc-data/x86_64-pc-linux-gnu/4.6.3/info --with-gxx-include-dir=/usr/lib/gcc/x86_64-pc-linux-gnu/4.6.3/include/g++-v4 --host=x86_64-pc-linux-gnu --build=x86_64-pc-linux -gnu --disable-altivec --disable-fixed-point --without-ppl --without-cloog --enable-lto --enable-nls --without-included-gettext --with-system-zlib --enable-obsolete --d isable-werror --enable-secureplt --enable-multilib --enable-libmudflap --disable-libssp --enable-libgomp --with-python-dir=/share/gcc-data/x86_64-pc-linux-gnu/4.6.3/pyt hon --enable-checking=release --disable-libgcj --enable-languages=c,c++,fortran --enable-shared --enable-threads=posix --enable-__cxa_atexit --enable-clocale=gnu --enab le-targets=all --with-bugurl=http://bugs.gentoo.org/ --with-pkgversion='Gentoo 4.6.3 p1.6, pie-0.5.2' Thread model: posix gcc version 4.6.3 (Gentoo 4.6.3 p1.6, pie-0.5.2) **************************************************** Deleting old versions of eclib libraries, which would interfere with new builds... Deleting old include directory... Now configuring eclib... configure: WARNING: unrecognized options: --with-gmp checking for a BSD-compatible install... /usr/bin/install -c checking whether build environment is sane... yes checking for a thread-safe mkdir -p... /bin/mkdir -p checking for gawk... gawk checking whether make sets $(MAKE)... yes configure: Configuring eclib... checking build system type... x86_64-unknown-linux-gnu checking host system type... x86_64-unknown-linux-gnu [...]
If you no longer depend on GMP/MPIR, you should remove the --with-gmp
option from spkg-install
and remove MPIR
from the dependencies in build/deps
and in SPKG.txt
.
comment:10 Changed 8 years ago by
John: you should update the dependencies in http://homepages.warwick.ac.uk/staff/J.E.Cremona/mwrank/index.html also.
comment:11 Changed 8 years ago by
The output of the saturation doctest (the one which changed) seems random. I have found
sage -t src/sage/schemes/elliptic_curves/ell_rational_field.py ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_rational_field.py", line 2163, in sage.schemes.elliptic_curves.ell_rational_field.EllipticCurve_rational_field.saturation Failed example: E.saturation([2*P]) # needs higher precision, handled by eclib Expected: ([(1755450733726721618440965414535034458701302721700399/970334851896750960577261378321772998240802013604 : -59636173615502879504846810677646864329901430096139563516090202443694810309127/955833935771565601591243078845907133814963790187832340692216425242529192 : 1)], 2, 113.302910926080) Got: ([(-439882683622326018487864747196375349103814452517687949/769190421485324305003142379600041098233439689147025 : 4502121973552688312422867845470844967444129306224713180310803904513790368504334/21332944386077194990255068064772535340283929319092661707597856317134035277625 : 1)], 2, 113.302910926080) **********************************************************************
and
sage -t --long src/sage/schemes/elliptic_curves/ell_rational_field.py ********************************************************************** File "src/sage/schemes/elliptic_curves/ell_rational_field.py", line 2163, in sage.schemes.elliptic_curves.ell_rational_field.EllipticCurve_rational_field.saturation Failed example: E.saturation([2*P]) # needs higher precision, handled by eclib Expected: ([(1755450733726721618440965414535034458701302721700399/970334851896750960577261378321772998240802013604 : -59636173615502879504846810677646864329901430096139563516090202443694810309127/955833935771565601591243078845907133814963790187832340692216425242529192 : 1)], 2, 113.302910926080) Got: ([(697059763916839811078337858798347162470013766225553/125882473941508715401055010125120306742453617664 : 17964093978634735025234271759384575412627262961292171402552454280120845611703/44663001336009603558781167435086158167457788739030704541177457005068288 : 1)], 2, 113.302910926080) **********************************************************************
comment:12 Changed 8 years ago by
A small detail: you could change See #10840
to See :trac:`10840`
.
comment:14 Changed 8 years ago by
- Branch changed from u/cremona/ticket/15738 to u/jdemeyer/ticket/15738
- Created changed from 01/26/14 16:30:39 to 01/26/14 16:30:39
- Modified changed from 01/27/14 15:59:19 to 01/27/14 15:59:19
comment:15 Changed 8 years ago by
- Commit changed from 315ab741adad959c8f43795ff0db7f9341c82023 to ded0c84c464bdecc63cd28a1ccbaa99c84c0ce6a
This should fix the doctest problem, but this still needs work to clarify the gmp dependency.
New commits:
ded0c84 | Fix problem with torsion points in saturation doctest
|
comment:16 follow-ups: ↓ 17 ↓ 18 Changed 8 years ago by
I'll look into all this... but gmp (or mpir) definitely is an indirect dependency (try ldd local/lib/libec.so or ldd local/bin/mwrank) but it is hidden in the sense that I explicitly use NTL and FLINT which themselves use gmp. Because of this I was told some time last year I should remove gmp from eclib's official dependency list, which I did but the install script did not change accordginly.
Peronally I think it is more useful to people if the dependencies are list of packages which need to be installed before building eclib, but I suppose if I really meant that I would need to include a lot more system libraries etc so that would be silly.
I will fix it! But I would like to find out why the doctest output is not deterministic first.
comment:17 in reply to: ↑ 16 Changed 8 years ago by
Replying to cremona:
I will fix it! But I would like to find out why the doctest output is not deterministic first.
Since the output differs by torsion points, mathematically it's still correct (see also my reviewer patch).
comment:18 in reply to: ↑ 16 Changed 8 years ago by
Replying to cremona:
I'll look into all this... but gmp (or mpir) definitely is an indirect dependency (try ldd local/lib/libec.so or ldd local/bin/mwrank) but it is hidden in the sense that I explicitly use NTL and FLINT which themselves use gmp. Because of this I was told some time last year I should remove gmp from eclib's official dependency list, which I did but the install script did not change accordginly.
Peronally I think it is more useful to people if the dependencies are list of packages which need to be installed before building eclib, but I suppose if I really meant that I would need to include a lot more system libraries etc so that would be silly.
It's hard to say what's best. Personally, I would vote to keep the GMP/MPIR dependency explicit and keep the --with-gmp
option as before. But for Sage it doesn't really matter, you just need to make spkg-install
and upstream consistent.
comment:19 Changed 8 years ago by
- Branch changed from u/jdemeyer/ticket/15738 to u/cremona/ticket/15738
- Commit changed from ded0c84c464bdecc63cd28a1ccbaa99c84c0ce6a to 13631f3545b1ec2c3b803e3cf1a819f3e0bf7613
- Description modified (diff)
- Status changed from needs_work to needs_review
New commits:
13631f3 | Trac 15738: remove eclibs's dependency on gmp; change to version 20140128.
|
comment:20 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_review to needs_work
- Summary changed from upgrade eclib to version 20140126 to upgrade eclib to version 20140128
My new commit adds to yours (u/jdemeyer/ticket/15738), bumps up the version number to today's (20140128) -- see https://github.com/JohnCremona/eclib/commit/3c19b3ae76f5b50c4e5374f5b561bad50dd58298 -- which updates the README to remove mention of non-existent gmp dependency and also make a small change to the source so that 2-division points are ordered deterministically.
I'll set to "needs review" again when I have done my own testing.
comment:21 Changed 8 years ago by
- Commit changed from 13631f3545b1ec2c3b803e3cf1a819f3e0bf7613 to bc292bace3dadf71ada95342098453d5f82b9f5e
Branch pushed to git repo; I updated commit sha1. New commits:
bc292ba | Trac 15738: update eclib checksum
|
comment:22 follow-up: ↓ 23 Changed 8 years ago by
- Status changed from needs_work to needs_review
OK, ready for review again. The last commit was necessary not because I had forgotten, but because I did actually change the upstream tarball. The one on sagemath is the right one -- note that I did change the version from 20140126 to 20140128 too.
I built this ok with SAGE_CHECK=yes and also ran all doctests including the long ones.
The self-test which eclib runs does not work perfectly when done with a lot of parallel threads owing (I presume) to race conditions. I had errors with MAKE set to "make -j50" but none without the -j50 tag. I think this was true before, so hope it will not be seen as a problem.
comment:23 in reply to: ↑ 22 ; follow-up: ↓ 24 Changed 8 years ago by
Replying to cremona:
The self-test which eclib runs does not work perfectly when done with a lot of parallel threads owing (I presume) to race conditions.
Are you open to bug reports about that? I have had a quick look and the problem seems to be that most programs are actually built twice in make check
(check the logs, you will see for example tmrank
is built twice, which can obviously be a race condition if that happens twice at the same time).
comment:24 in reply to: ↑ 23 ; follow-up: ↓ 25 Changed 8 years ago by
Replying to jdemeyer:
Replying to cremona:
The self-test which eclib runs does not work perfectly when done with a lot of parallel threads owing (I presume) to race conditions.
Are you open to bug reports about that? I have had a quick look and the problem seems to be that most programs are actually built twice in
make check
(check the logs, you will see for exampletmrank
is built twice, which can obviously be a race condition if that happens twice at the same time).
Of course I am open to bug reports! It's up to you whether you think they are reasons not to include this version in Sage. But if easily fixed, now would be a good time to fix them.
I know of no reason ahy any test program should be built twice. If I delete etsts/tmrank and type "make tmrank" I see this:
/bin/bash ../libtool --tag=CXX --mode=link g++ -g -O2 -L/usr/local/lib -L/usr/lib -o tmrank tmrank.o ../libsrc/libec.la -lflint -lpari -lntl -lboost_system-mt -lboost_thread libtool: link: g++ -g -O2 -o .libs/tmrank tmrank.o -L/usr/local/lib -L/usr/lib ../libsrc/.libs/libec.so -lflint -lpari /usr/local/lib/libntl.so -lboost_system-mt -lboost_thread -Wl,-rpath -Wl,/home/jec/eclib-parallel/lib
but surely that is only one build?
Apart from building, some of the tests will only work properly if the are run in the correct sequence since one writes a temporary files which another needs to read. So far this has worked ok, but it would be possible to change the target check_g0n
in tests/Makefile.am to forece them to run in order. That would be easy, in fact. None of the other check targets have this defect.
comment:25 in reply to: ↑ 24 Changed 8 years ago by
Apart from building, some of the tests will only work properly if the are run in the correct sequence since one writes a temporary files which another needs to read. So far this has worked ok, but it would be possible to change the target
check_g0n
in tests/Makefile.am to forece them to run in order. That would be easy(*), in fact. None of the other check targets have this defect.
(*) Not quite so easy: in tests/ the only dependency is to run tmanin before nftest and oftest. But in progs/ there are more dependencies, and I am in the process of working out exactly what they are,
comment:26 Changed 8 years ago by
Within every test target (e.g. "check_procs"), tests are run in order because it's just a shell for
loop. In particular, modtest homtest hecketest mhcount tmanin nftest oftest tnfd
are run in this order.
comment:27 Changed 8 years ago by
- Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.
- Status changed from needs_review to needs_work
Fix for make check
reported upstream, waiting for new upstream version.
comment:28 Changed 8 years ago by
- Commit changed from bc292bace3dadf71ada95342098453d5f82b9f5e to 220c50e635fc85fb24b85378c87b37fbf01567b4
Branch pushed to git repo; I updated commit sha1. New commits:
220c50e | updated eclib checksum after patching upstream
|
comment:29 Changed 8 years ago by
- Status changed from needs_work to needs_review
I patched the upstream -- see here: https://github.com/JohnCremona/eclib/commit/f9441d07400b7736004209dfcbfdf3ce87e84e75
The tarball at sagemath has been updated (same name though) and the new commit just updates the checksum.
comment:30 Changed 8 years ago by
- Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, in a later stable release.
- Status changed from needs_review to positive_review
comment:31 Changed 8 years ago by
Thanks a lot -- especially for the help with my build/check script!
comment:32 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:33 Changed 8 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Trac 15738: update eclib to version 20140126