update eclib and improve interface for elliptic curve saturation

JohnCremona commented 3 years ago

I recently made a new version of eclib, which will only affect the saturation of points on elliptic curves over Q. The interface for this (in the saturation() method for elliptic curves over Q) has some problems. Mainly, it does not do a good job of choosing the real precision which mwrank should work at, and also (due to shortcomings in eclib which are now improved) (1) when saturation at one prime fails owing to precision being too low, Sage has no way to do something sensible such as raise an error, or put the call into a loop in which the precision is increased; (2) there is no way to saturate at a single prime, or a range of primes, only at all primes or all primes except 2 or all primes up to some bound. The new eclib version allows for a first and last prime at which saturation should be done. The rationale here is that if you have saturated points on one curve and map them to another using an isogeny, then the image points will automatically be saturated at all primes dividing the isogeny degree.

I don't see a sensible way to deal with this in two parts, so I'll make a new eclib spkg (if that is what they are still called) and make the code changes to saturation at the same time.

Incidentally, Sage has its own implementation of the same underlying algorithm for elliptic curves over number fields, as as well as all the above, there should be a parameter called 'implementation' which could be either 'sage' or 'eclib' (or 'pari' before long).

New eclib source tarball at https://github.com/JohnCremona/eclib/releases/download/20210625/eclib-20210625.tar.bz2

Upstream: Fixed upstream, in a later stable release.

CC: @slel @collares @orlitzky @isuruf @kiwifb @antonio-rojas

Component: packages: standard

Keywords: eclib elliptic curve saturation

Author: John Cremona, Dima Pasechnik

Branch: 789550c

Reviewer: Dima Pasechnik, Matthias Koeppe

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

slel commented 3 years ago

comment:40

Maybe ./configure --with-mp=gmp can help?

JohnCremona commented 3 years ago

Description changed:

--- 
+++ 
@@ -4,4 +4,4 @@

 Incidentally, Sage has its own implementation of the same underlying algorithm for elliptic curves over number fields, as as well as all the above, there should be a parameter called 'implementation' which could be either 'sage' or 'eclib' (or 'pari' before long).

-New eclib source tarball at [https://johncremona.github.io/ftp/eclib-v20210317.tar.bz2](https://johncremona.github.io/ftp/eclib-v20210317.tar.bz2)
+New eclib source tarball at [https://johncremona.github.io/ftp/eclib-20210318.tar.bz2](https://johncremona.github.io/ftp/eclib-20210318.tar.bz2)

dimpase commented 3 years ago

Description changed:

--- 
+++ 
@@ -4,4 +4,4 @@

 Incidentally, Sage has its own implementation of the same underlying algorithm for elliptic curves over number fields, as as well as all the above, there should be a parameter called 'implementation' which could be either 'sage' or 'eclib' (or 'pari' before long).

-New eclib source tarball at [https://johncremona.github.io/ftp/eclib-20210318.tar.bz2](https://johncremona.github.io/ftp/eclib-20210318.tar.bz2)
+New eclib source tarball at [https://johncremona.github.io/ftp/eclib-v20210317.tar.bz2](https://johncremona.github.io/ftp/eclib-v20210317.tar.bz2)

dimpase commented 3 years ago

comment:42

Replying to @JohnCremona: eclib itself detects gmp, and when I run ldd on my latest version of the shared libec library I see

libgmp.so.10 => /usr/lib/x86_64-linux-gnu/libgmp.so.10 (0x00007f23ee737000)
so I don't know why Sage's configure does not find it. I do build mpir in its gmp-compat mode, so as well as the system library as above, in /usr/local/lib I have both libmpir and libgmp both installed when I do 'make install' on mpir.

Sage most probably finds mpir in /usr/local first, and then rejects it for some reason. Please post top-level config.log (This is a configuration noone tried since few years, I guess. MPIR is semi-abandoned, I don't know why we have not downgraded it to experimental yet)

JohnCremona commented 3 years ago

comment:43

I wil try that configure suggestion, thanks.

Meanwhile I have updated the branch with yet another version of the tarball, checksums etc, 20210318 (no v) but for some reason I cannot now push to trac, either to u/cremona/31443-eclib-saturation (by old branch) or public/packages/eclib/31443. In both cases I see

fatal: remote error: access denied or repository not exported: /sage.git

I have changed nothing with my ssh setup (no new keys, etc). Suggestions?

JohnCremona commented 3 years ago

Description changed:

--- 
+++ 
@@ -4,4 +4,4 @@

 Incidentally, Sage has its own implementation of the same underlying algorithm for elliptic curves over number fields, as as well as all the above, there should be a parameter called 'implementation' which could be either 'sage' or 'eclib' (or 'pari' before long).

-New eclib source tarball at [https://johncremona.github.io/ftp/eclib-v20210317.tar.bz2](https://johncremona.github.io/ftp/eclib-v20210317.tar.bz2)
+New eclib source tarball at [https://johncremona.github.io/ftp/eclib-20210318.tar.bz2](https://johncremona.github.io/ftp/eclib-20210318.tar.bz2)

JohnCremona commented 3 years ago

comment:44

Replying to @dimpase:

Replying to @JohnCremona: eclib itself detects gmp, and when I run ldd on my latest version of the shared libec library I see
libgmp.so.10 => /usr/lib/x86_64-linux-gnu/libgmp.so.10 (0x00007f23ee737000)
so I don't know why Sage's configure does not find it. I do build mpir in its gmp-compat mode, so as well as the system library as above, in /usr/local/lib I have both libmpir and libgmp both installed when I do 'make install' on mpir.
Sage most probably finds mpir in /usr/local first, and then rejects it for some reason. Please post top-level config.log (This is a configuration noone tried since few years, I guess. MPIR is semi-abandoned, I don't know why we have not downgraded it to experimental yet)

I am not tied at all to MPIR, I started using it when Bill Hart was in Warwick. But it will simplify m life a bit to just use ordinary gmp, so I will uninstall mpir and hope that this will fix things.

dimpase commented 3 years ago

comment:45

do you by any chance use git trac extension?

JohnCremona commented 3 years ago

comment:46

Replying to @dimpase:

do you by any chance use git trac extension?

No

dimpase commented 3 years ago

comment:47

what do you get if you run ssh git@trac.sagemath.org ? You'd see something like

hello <username>, this is git@trac running gitolite3 3.5.3.1-2 (Debian) on git 1.9.1

 R W    sage
Connection to trac.sagemath.org closed.

JohnCremona commented 3 years ago

comment:48

Replying to @dimpase:

what do you get if you run ssh git@trac.sagemath.org ? You'd see something like
hello <username>, this is git@trac running gitolite3 3.5.3.1-2 (Debian) on git 1.9.1

 R W  sage
Connection to trac.sagemath.org closed.

I get this:

$ ssh git@trac.sagemath.org
X11 forwarding request failed on channel 0
PTY allocation request failed on channel 0
hello cremona, this is git@trac running gitolite3 3.5.3.1-2 (Debian) on git 1.9.1

 R W    sage
Connection to trac.sagemath.org closed.

which looks OK to me.

JohnCremona commented 3 years ago

comment:49

When I uninstalled MPIR I managed to so seriously break my system that gcc did not work. I even reinstalled ubuntu's gmp and gcc but they still did not work. So I reinstalled MPIR, which puts libgmp files (as well as libmpir files) into /usr/local, and now gcc works. Bizarre.

dimpase commented 3 years ago

comment:50

What does git remote -v say for this repo?

dimpase commented 3 years ago

comment:51

Replying to @JohnCremona:

When I uninstalled MPIR I managed to so seriously break my system that gcc did not work. I even reinstalled ubuntu's gmp and gcc but they still did not work. So I reinstalled MPIR, which puts libgmp files (as well as libmpir files) into /usr/local, and now gcc works. Bizarre.

gmp is a build dependency of gcc, so not too bizzare.

However, system packages should not depend on anything in /usr/local, so perhaps something was not properly flushed...

JohnCremona commented 3 years ago

comment:52

Replying to @dimpase:

What does git remote -v say for this repo?

$ git remote -v
trac    git://trac.sagemath.org/sage.git (fetch)
trac    git://trac.sagemath.org/sage.git (push)

This is a repo I downloaded yesterday, have made some changes to (in build/ and whatever else the branch on this ticket touches such as src/libs/eclib) but have never actually built, only run ./bootstrap and ./configure.

./configure does now report that it can use the system gmp -- this is only true after rerunning ./bootstrap and adding --with-mp=pir on the configure command line. That is progress. But it still does not like my NTL. Here's a bit of config.log:

## ----------------------------------------------------- ##
## Checking whether SageMath should install SPKG mpir... ##
## ----------------------------------------------------- ##
configure:10351: no suitable system package found for SPKG mpir
configure:10371: result: using mpir SPKG (via --with-mp=mpir)
## ---------------------------------------------------- ##
## Checking whether SageMath should install SPKG gmp... ##
## ---------------------------------------------------- ##
configure:10526: will use system package and not install SPKG gmp
## ----------------------------------------------------- ##
## Checking whether SageMath should install SPKG mpfr... ##
## ----------------------------------------------------- ##
configure:10624: checking installing gmp/mpir? 
configure:10627: result: yes; install mpfr as well
configure:10715: no suitable system package found for SPKG mpfr
configure:10733: result: using Sage's mpfr SPKG
## ---------------------------------------------------- ##
## Checking whether SageMath should install SPKG ntl... ##
## ---------------------------------------------------- ##
configure:10814: checking installing gmp/mpir? 
configure:10817: result: yes; install ntl as well
configure:10929: no suitable system package found for SPKG ntl
## ------------------------------------------------------ ##
## Checking whether SageMath should install SPKG flint... ##
## ------------------------------------------------------ ##
configure:11136: checking installing mpfr or ntl? 
configure:11139: result: yes; install flint as well
configure:11324: no suitable system package found for SPKG flint
configure:11342: result: using Sage's flint SPKG
## ---------------------------------------------------- ##
## Checking whether SageMath should install SPKG arb... ##
## ---------------------------------------------------- ##
configure:11424: checking installing flint? 
configure:11427: result: yes; install arb as well
configure:11544: no suitable system package found for SPKG arb

This is so inconsistent!

JohnCremona commented 3 years ago

comment:53

Replying to @dimpase:

Replying to @JohnCremona:

When I uninstalled MPIR I managed to so seriously break my system that gcc did not work. I even reinstalled ubuntu's gmp and gcc but they still did not work. So I reinstalled MPIR, which puts libgmp files (as well as libmpir files) into /usr/local, and now gcc works. Bizarre.

gmp is a build dependency of gcc, so not too bizzare.

I think it is bizarre since I reinstalled the system gmp and gcc and after that gcc did not work, while after reinstalling my own mpir build gcc does work.

However, system packages should not depend on anything in /usr/local, so perhaps something was not properly flushed...

I don't know exactly what that means or how to fix it.

The only reason I am in this mess is that I was trying to improve Sage for Sage users by improving eclib (no-one else uses it). I have now spent several days on all this package installation nonsense, which used to be very simple but is now in some in-between state where things only half work. And now I cannot even push my branch to trac for some other stupid reason. I did push it to my github fork https://github.com/JohnCremona/sage/tree/31443-eclib-saturation

but I have had enough of all this. I may come back to it next week, but I have others things to do.

dimpase commented 3 years ago

comment:54

Replying to @JohnCremona:

Replying to @dimpase:

What does git remote -v say for this repo?
$ git remote -v
trac  git://trac.sagemath.org/sage.git (fetch)
trac  git://trac.sagemath.org/sage.git (push)

remove these git:// things - you are not using ssh while pushing to a git:// URL. git:// is a "native" git protocol that does authentication via a password, I have no idea how you actually managed to push from this machine earlier. That is, run

git remote rm trac
git remote add trac git@trac.sagemath.org:sage.git

I think it is bizarre since I reinstalled the system gmp and gcc and after that gcc did not work, while after reinstalling my own mpir build gcc does work.

However, system packages should not depend on anything in /usr/local, so perhaps something was not properly flushed...

I don't know exactly what that means or how to fix it.

I don't know either, I've never seen this before --- I only know that your gcc toolchain is messed up (and this doesn't have much to do with Sage). It might be as simple as looging out and back in, or perhaps a reboot, bit it really no Sage's fault.

And now I cannot even push my branch to trac for some other stupid reason.

which this is not Sage's fault (other that Sage messes up with MPIR for far too long :-)).

I think your packages is in a good shape, I will finish up the branch and put it up for review.

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

Changed commit from 573bcf6 to 926670f

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

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

`7db4910`	`remove dud empty file`
`28408fc`	`correct upstream_url for eclib`
`d32295f`	`fix spkg-configure for eclib`
`5a90432`	`updated eclib build files to 20210318`
`926670f`	`Merge remote-tracking branch 'jc/31443-eclib-saturation' into public/packages/eclib/31443`

JohnCremona commented 3 years ago

comment:56

Thanks to everyone for their patience. I have no idea how I got the trac remote set up so weirdly on this new clone. On my main sage clone it looked like

trac    git://trac.sagemath.org/sage.git (fetch)
trac    git@trac.sagemath.org:sage.git (push)

which had been working, apparantly, but I have now sorted that one out too. So I could finally push my branch to u/cremona/31443-eclib-saturation though that was made reduandant thanks to Dima's help.

dimpase commented 3 years ago

comment:57

Replying to @JohnCremona:

Thanks to everyone for their patience. I have no idea how I got the trac remote set up so weirdly on this new clone. On my main sage clone it looked like
trac  git://trac.sagemath.org/sage.git (fetch)
trac  git@trac.sagemath.org:sage.git (push)
which had been working,

this is fine - git:// works for fetch, this does not need authentication, and the push url is OK here, it is an ssh one.

git:// sometimes does not work through firewalls (if they are not too well set up)

dimpase commented 3 years ago

Reviewer: Dima Pasechnik

dimpase commented 3 years ago

Changed author from John Cremona to John Cremona, Dima Pasechnik

dimpase commented 3 years ago

comment:58

this looks good,perhaps someone who knowns autoconf can review my changes to spkg-configure.m4

mkoeppe commented 3 years ago

comment:59

Replying to @JohnCremona:

I think it would be quite reasonable to insist that the version installed on the system is exactly the version specified, not just "this or any later version", since I have enough to worry about without having to think about backwards compatibility. I'm also fairly sure that there is no-one in the world (except me) who installs eclib except as part of Sage anyway.

There is also a possible middle ground - you could set an upper bound as well such as <= 20231231.

Does the new version of eclib work with old version of Sage?

Downstream distributors of sage could perhaps comment whether fixing the eclib version creates additional burden for them.

antonio-rojas commented 3 years ago

comment:60

Replying to @mkoeppe:

Downstream distributors of sage could perhaps comment whether fixing the eclib version creates additional burden for them.

I don't use sage's build system, so personally I don't care which eclib version the m4 file expects. I always make sure that the versions of eclib and sagelib in our repos are compatible.

JohnCremona commented 3 years ago

comment:61

No, the new version of eclib will not work with older sages: the sage code changes here in the eclib interface are necesary.

I just push a tag '20210318' to eclib on github and will make a release with the same name.

From what I have been discovering about dependencies in Sage's own build system, since Sage has a patched version of pari it will always build any package with pari as a dependency. But eclib works fine with the latest stable release of pari (2.11.2) so I don't think that distro package managers will be bothered: they can upgrade their eclib to 20210318 at the same time as upgrading sage to 9.3, assuming that this ticket is merged into 9.3.

kiwifb commented 3 years ago

comment:62

I can pin a version for sage-9.2 and a different one for beta and future 9.3 in Gentoo.

dimpase commented 3 years ago

comment:63

Replying to @JohnCremona:

No, the new version of eclib will not work with older sages: the sage code changes here in the eclib interface are necesary.

I just push a tag '20210318' to eclib on github and will make a release with the same name.

From what I have been discovering about dependencies in Sage's own build system, since Sage has a patched version of pari it will always build any package with pari as a dependency.

no, this is not correct. There are versions of Pari 2.11 around on some Linux distributions which are accepted by Sage and may be used. Sage's patches to Pari are needed for Cygwin.

JohnCremona commented 3 years ago

comment:64

By the way, about eclib's 3 dependencies:

NTL is vital, used in many ways throughout.
FLINT is optional -- eclib can be configured and built without it, though I am not sure when I tested that. The only difference would be in the speed of certain parts of linear algebra mod p which are used by Sage only in some modular symbols code. That would still work without the FLINT use but larger examples (large enough to take hours anyway) would be slower. It will be easy to write that using NTL instead, which I will do and if the performance is no worse than using FLINT then I will consider removing the FLINT version and hance that dependency.
pari could be made optional, though there seems little point in that. Until version 20190909, the only difference would have been that without it integer factorization would only use trial division. Since the 2021 releases eclib also uses libpari's ellap function for point-counting on elliptic curves mod p. I could make that optional too since eclib can do its own point counting (it is just slower).

mkoeppe commented 3 years ago

comment:65

Replying to @JohnCremona:

I just push a tag '20210318' to eclib on github and will make a release with the same name.

You may want to put the release tarball there.

JohnCremona commented 3 years ago

comment:66

Replying to @mkoeppe:

Replying to @JohnCremona:

I just push a tag '20210318' to eclib on github and will make a release with the same name.

You may want to put the release tarball there.

Github makes a tarball whenever one makes a release, and it is at https://github.com/JohnCremona/eclib/releases/tag/20210318 but I don't think that it is identical in content to what one gets from autotools "make dist". I would certainly prefer to having the tarball there instead of making a separate one and putting it on my own web page.

mkoeppe commented 3 years ago

comment:67

That's right. What GitHub puts there is not a proper release tarball; it is merely an archive of what's in the repository.

You can use the "edit" button on the release page to upload the actual release tarball there.

JohnCremona commented 3 years ago

comment:68

Replying to @mkoeppe:

That's right. What GitHub puts there is not a proper release tarball; it is merely an archive of what's in the repository.

You can use the "edit" button on the release page to upload the actual release tarball there.

Ah I didn't know that . Thanks, I will do. Then Ill need to change vhecksums.ini again with the changed URL.

mkoeppe commented 3 years ago

comment:69

Replying to @JohnCremona:

about eclib's 3 dependencies:

Thanks for sharing this information. At the moment, and also for the Sage 9.4 release cycle (#29705), all of these three libraries are essential standard components of Sage; so from the viewpoint of Sage there is no immediate need to worry about making any of these dependencies optional.

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

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

`5d9e70a`	`#31443: updated upstream tarball URL`
`8b64fed`	`Merge remote-tracking branch 'trac/public/packages/eclib/31443' into 31443-eclib-saturation`

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

Changed commit from 926670f to 8b64fed

JohnCremona commented 3 years ago

comment:71

I put the proper release tarball onto github and updated both the link on this ticket and the last line of checksums.ini. The latter now reads

upstream_url=https://github.com/JohnCremona/eclib/releases/download/VERSION/eclib-VERSION.tar.bz2

with "VERSION" twice since the version string "20210318" appears twice in the URL. I hope that Sage can handle that.

JohnCremona commented 3 years ago

Description changed:

--- 
+++ 
@@ -4,4 +4,5 @@

 Incidentally, Sage has its own implementation of the same underlying algorithm for elliptic curves over number fields, as as well as all the above, there should be a parameter called 'implementation' which could be either 'sage' or 'eclib' (or 'pari' before long).

-New eclib source tarball at [https://johncremona.github.io/ftp/eclib-20210318.tar.bz2](https://johncremona.github.io/ftp/eclib-20210318.tar.bz2)
+New eclib source tarball at 
+[https://github.com/JohnCremona/eclib/releases/download/20210318/eclib-20210318.tar.bz2](https://github.com/JohnCremona/eclib/releases/download/20210318/eclib-20210318.tar.bz2)

JohnCremona commented 3 years ago

comment:72

Replying to @mkoeppe:

Replying to @JohnCremona:

about eclib's 3 dependencies:

Thanks for sharing this information. At the moment, and also for the Sage 9.4 release cycle (#29705), all of these three libraries are essential standard components of Sage; so from the viewpoint of Sage there is no immediate need to worry about making any of these dependencies optional.

I realize that Sage itself will carry on using FLINT, but it would help me not to have to build it on machines I use, and also it would make it slightly more likely that someone building Sage would not need to build eclib.

For the record, I now have a branch which uses an NTL function instead of eclib's own for reduction of dense matrices modulo a small (int or long) prime p when FLINT_LEVEL=0 -- i.e. for when FLINT is not available, or configure has been given --without-flint. Testing shows that this is reasonable -- much better than using eclib's own naive function -- though slower than using FLINT. So it will be there in some future release, which might make without-flint the default, but there is no urgency.

mkoeppe commented 3 years ago

Changed reviewer from Dima Pasechnik to Dima Pasechnik, https://github.com/mkoeppe/sage/actions/runs/680630789

mkoeppe commented 3 years ago

Changed reviewer from Dima Pasechnik, https://github.com/mkoeppe/sage/actions/runs/680630789 to Dima Pasechnik, Matthias Koeppe

mkoeppe commented 3 years ago

comment:74

The tests at https://github.com/mkoeppe/sage/actions/runs/680630789 have run successfully. LGTM.

JohnCremona commented 3 years ago

comment:75

Thanks to all for the reviewing and other help.

JohnCremona commented 3 years ago

comment:76

I found a bug in some eclib code which has the effect of making the "saturation index bound" much larger than necessary. In the example I mentioned above (Comment 3) the bound is no longer ~86000 but ony ~11800, taking the time to saturate that example down from 15 minutes (which was many hours before I started) to under a minute.

What this means is that is that there will be yet another eclib release soon (I am hoping for tomorrow), so I am quite relaxed about whether this version gets into 9.3, despite having a positive review a week ago.

By the way, the bug was found while comparing results with Bill Allombert (pari developer) who is implementing the same stuff in pari/gp. By the time he has finished and newer versions of pari are in Sage, Sage will hardly need eclib at all...hooray!

mkoeppe commented 3 years ago

comment:77

Moving this ticket to 9.4, as it seems unlikely that it will be merged in 9.3, which is in the release candidate stage

JohnCremona commented 3 years ago

comment:78

I am about to update this with eclib version 20210408.

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

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

`17e91ca`	`Merge remote-tracking branch 'trac/develop' into 31443-eclib-saturation`
`f18fa63`	`Merge remote-tracking branch 'trac/develop' into 31443-eclib-saturation`
`f564931`	`updated eclib package version to 20210408 and checksums`
`07c5a20`	`correct a docstring after eclib upgrade`
`8c4c115`	`Merge branch 'public/packages/eclib/31443' of trac.sagemath.org:sage into 31443-eclib-saturation`

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

Changed commit from 8b64fed to 8c4c115

JohnCremona commented 3 years ago

Description changed:

--- 
+++ 
@@ -5,4 +5,4 @@
 Incidentally, Sage has its own implementation of the same underlying algorithm for elliptic curves over number fields, as as well as all the above, there should be a parameter called 'implementation' which could be either 'sage' or 'eclib' (or 'pari' before long).

 New eclib source tarball at 
-[https://github.com/JohnCremona/eclib/releases/download/20210318/eclib-20210318.tar.bz2](https://github.com/JohnCremona/eclib/releases/download/20210318/eclib-20210318.tar.bz2)
+[https://github.com/JohnCremona/eclib/releases/download/20210408/eclib-20210318.tar.bz2](https://github.com/JohnCremona/eclib/releases/download/20210408/eclib-20210318.tar.bz2)

sagemath / sage

update eclib and improve interface for elliptic curve saturation #31443