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:

Status badges

Description (last modified by cremona)

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 cremona

  • Description modified (diff)

comment:2 Changed 8 years ago by cremona

  • Branch set to u/cremona/ticket/15738
  • Commit set to 32af9323edfbe7f2e954c8fcf13867c4f8a2c153
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

32af932Trac 15738: update eclib to version 20140126

comment:3 Changed 8 years ago by cremona

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

  • Description modified (diff)

comment:5 follow-up: Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

You should add a == License == section to SPKG.txt.

comment:6 Changed 8 years ago by jdemeyer

Builds fine with SAGE_CHECK=yes.

comment:7 Changed 8 years ago by git

  • Commit changed from 32af9323edfbe7f2e954c8fcf13867c4f8a2c153 to 315ab741adad959c8f43795ff0db7f9341c82023

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

315ab74Added License section to eclib's SPKG.txt

comment:8 in reply to: ↑ 5 Changed 8 years ago by cremona

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

You should add a == License == section to SPKG.txt.

Done. Thanks for reviewing.

comment:9 Changed 8 years ago by jdemeyer

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

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 jdemeyer

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 jdemeyer

A small detail: you could change See #10840 to See :trac:`10840`.

comment:13 Changed 8 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

Working on reviewer patch...

comment:14 Changed 8 years ago by jdemeyer

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

  • Commit changed from 315ab741adad959c8f43795ff0db7f9341c82023 to ded0c84c464bdecc63cd28a1ccbaa99c84c0ce6a

This should fix the doctest problem, but this still needs work to clarify the gmp dependency.


New commits:

ded0c84Fix problem with torsion points in saturation doctest

comment:16 follow-ups: Changed 8 years ago by 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.

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 jdemeyer

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 jdemeyer

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 cremona

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

13631f3Trac 15738: remove eclibs's dependency on gmp; change to version 20140128.

comment:20 Changed 8 years ago by cremona

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

  • Commit changed from 13631f3545b1ec2c3b803e3cf1a819f3e0bf7613 to bc292bace3dadf71ada95342098453d5f82b9f5e

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

bc292baTrac 15738: update eclib checksum

comment:22 follow-up: Changed 8 years ago by cremona

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

Last edited 8 years ago by cremona (previous) (diff)

comment:23 in reply to: ↑ 22 ; follow-up: Changed 8 years ago by 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 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: Changed 8 years ago by cremona

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 example tmrank 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 cremona

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 jdemeyer

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 jdemeyer

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

  • Commit changed from bc292bace3dadf71ada95342098453d5f82b9f5e to 220c50e635fc85fb24b85378c87b37fbf01567b4

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

220c50eupdated eclib checksum after patching upstream

comment:29 Changed 8 years ago by cremona

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

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

Thanks a lot -- especially for the help with my build/check script!

comment:32 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:33 Changed 8 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.