Opened 15 months ago

Closed 11 months ago

Last modified 11 months ago

#31443 closed enhancement (fixed)

update eclib and improve interface for elliptic curve saturation

Reported by: cremona Owned by:
Priority: major Milestone: sage-9.4
Component: packages: standard Keywords: eclib elliptic curve saturation
Cc: slelievre, gh-collares, mjo, isuruf, fbissey, arojas Merged in:
Authors: John Cremona, Dima Pasechnik Reviewers: Dima Pasechnik, Matthias Koeppe
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 789550c (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by cremona)

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

Change History (128)

comment:1 Changed 15 months ago by slelievre

  • Cc slelievre added
  • Description modified (diff)

comment:2 Changed 15 months ago by cremona

  • Authors set to John Cremona
  • Branch set to u/cremona/31443-eclib-saturation
  • Commit set to 69c3195d7adee86991ae3b9a47081f3222178d18
  • Description modified (diff)
  • Keywords eclib elliptic curve saturation added
  • Status changed from new to needs_review

Here's the updated eclib version v20210310 (see tarball link above) together with the necessary changes to the interface in src/sage/libs/eclib, and also a much simplified (and improved) saturation() method in sage/schemes/elliptic_curves/ell_rational_field.

This was a symbiotic procedure, in the sense that as I modified the interface code and tested, I found issues which had to be fixed on the eclib side.

The main changes for eclib are: various efficiency improvements in the sieve used to p-saturation for an individual prime p; much better division of a point by p (when it is p times a smaller point), by default using division polynomials instead of floating point methods, though I did also improve the f.p. approach to set the precision appropriately (instead of just relying on the user to do so) which fixed the immediate hard cases I had; reducing the saturation index bound when the subgroup is enlarged. I also made some improvements to the construction of a newform from an elliptic curve which will avoid the normalisation problems with modular symbols (which I had already fixed on the Sage side, but this is more robust).

I will post here some elliptic curves whose saturation is now faster and/or better. There are plenty of examples in the old doctests, but the really hard cases are not very suitable for doctests as they involve hundreds (or thousands) of digits, or take 15 minutes.


New commits:

2215893#31443: update eclib package metadata
2554967#31443: update eclib package metadata again
e1dfacf#31443: adapt eclib library interface for version v20210310
69c3195#31443: simplify/improve saturation method for elliptic curves over Q

comment:3 Changed 15 months ago by cremona

An example where the point P is 3*Q but E.saturation([P]) used to fail.

In Sage 9.2:

E = EllipticCurve([0, 1, 0, -3532341, 2671895459])
Q = E([188751344014/116704809 , 43530702836852015/1260762051627])
P = 3*Q
E.saturation([P]) # fails

even after mwrank_set_precision(2000), which is slow. But with the new version (and no precision adjustment necessary, as the new method is exact):

sage: E.saturation([P])                                                                                                                               
([(188751344014/116704809 : 43530702836852015/1260762051627 : 1)],
 3,
 22.2351273597058)

and this is instantaneous.

An example you will probably not want to run to completion:

sage: E = EllipticCurve([0,1,1,-9872,374262])                                                                                                         
sage: pts = E.gens(sat_bound=0)                                                                                                                       
sage: pts                                                                                                                                             
[(-88 : 754 : 1), (-42 : 846 : 1), (-25 : 778 : 1), (57 : -1 : 1)]
sage: %time E.saturation(pts, verbose=True)

Under 9.2 it reports that the saturation index bound is 86682 but that it will not go past 10000 -- you can override this if you want, but even going up to 10000 takes a long time. With the new code, it does not reduce the bound (since the default is supposed to be that it goes all the way up to the bound necessary to prove saturation) and takes about 15 minutes.

comment:4 Changed 15 months ago by cremona

In fact the second example is even more stupid in 9.2 since after proving p-saturation at all p up to 10979, with no errors or problems at all, it adds 1000 to the bound and starts again proving p-saturation from 2 up to 11981. As each of these runs takes several minutes, it will take a long time to get up to 86682 this way. One issue (now addressed) is that eclib did not have a parameter specifying which p to start at, if you wanted to resume saturating at any point except 2 or 3 (there was only an option to skip p=2). But more seriously, the saturation was being repeated not because of any problem which has arisen, just because of the artificial lowering of the bound. The old version could do the job by setting max_prime to 86682 but that takes a lot longer than the new version.

comment:5 Changed 15 months ago by gh-collares

  • Cc gh-collares added

comment:6 Changed 14 months ago by arojas

  • Status changed from needs_review to needs_work

Needs rebasing

comment:7 Changed 14 months ago by git

  • Commit changed from 69c3195d7adee86991ae3b9a47081f3222178d18 to 4ba267ac16722b3eee43d2b4ed73840c366a8bec

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

1f0b74f#31443: update eclib package metadata
5fce79f#31443: update eclib package metadata again
a46b6f9#31443: adapt eclib library interface for version v20210310
453c793#31443: simplify/improve saturation method for elliptic curves over Q
4ba267a#31443: fix two doctest output now that eclib no longer gives incorrect results

comment:8 Changed 14 months ago by cremona

  • Status changed from needs_work to needs_review

I rebased, fixed a merge conflict and two doctest outputs which were there to illustrate how it used to be true that you could get wrong results from eclib by choosing a parameter too small, but now eclib adjusts itself automatically so such an illustration is not possible.

comment:9 follow-up: Changed 14 months ago by mkoeppe

checksums.ini needs a new field upstream_url, as explained in https://wiki.sagemath.org/ReleaseTours/sage-9.1#For_developers-1

comment:10 in reply to: ↑ 9 Changed 14 months ago by cremona

Replying to mkoeppe:

checksums.ini needs a new field upstream_url, as explained in https://wiki.sagemath.org/ReleaseTours/sage-9.1#For_developers-1

Thanks, I will look at that tomorrow. I did re-read the packaging instructions to refresh my memory and found some new stuff, but the contents of checksums.ini is whatever was put there by whatever the instructions told me to do. (I have been updating the eclib package since about 2008...)

comment:11 Changed 14 months ago by mkoeppe

(The packaging documentation has been updated in #30895 during the 9.3 cycle; it's not in the online copy of the Sage documentation yet.)

comment:12 Changed 14 months ago by cremona

I added the line (literally) upstream_url=https://github.com/JohnCremona/eclib/archive/VERSION.tar.gz to build/pkgs/eclib/checksums.ini and then did ./sage --package fix-checksum eclib though the response was Checksum of eclib (tarball eclib-20210310.tar.bz2) unchanged

I'll update the branch in a minute, but I suspect that Sage will not like the fact that the upstream tarball (which is a tar.gz file created by github) is not the same as the tarball I put into upstream/ (and in the link on this ticket) which was created using make dist-bzip2.

If they need to be identical then I'll replace the actual tarball and then the checksum really will change.

The developers guide instructions are not at all clear. For example, I had assumed that one was suppose to replace the string 'VERSION' with the actual version number. Or is it supposed to be literally the string 'VERSION' which will be replaced by the content of the file package-version.txt? If that is the case I'll need to make another adjustment.

comment:13 Changed 14 months ago by git

  • Commit changed from 4ba267ac16722b3eee43d2b4ed73840c366a8bec to f4752618381fc589d3b992d67cd29e9a9c7086d5

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

f475261added upstream_url to eclib's checksums.ini

comment:14 Changed 14 months ago by cremona

Now I also did what I thought I should have done, namely change package-version.txt to be v20210310' (before it did not have the v but github puts the v in as the relevant tag is v20210310), and change the first line of checksum.ini to tarball=eclib-VERSION.tar.gz. I put an actual download from github of the tarball into upstream/; since github calls the file just v20210310.tar.gz I added the prefix 'eclib-'. Then

./sage --package fix-checksum eclib

did change the checksums. I then did

./sage -package update eclib v20210310

and then

./sage -f eclib

but this failed quickly with

[eclib-v20210310] Package 'eclib' is currently not installed
[eclib-v20210310] No legacy uninstaller found for 'eclib'; nothing to do
[eclib-v20210310] Deleting old versions of eclib libraries, which
[eclib-v20210310] would interfere with new builds...
[eclib-v20210310] Deleting old include directory...
[eclib-v20210310] --with-flint=/home/john/sage/local
[eclib-v20210310] Configuring eclib-v20210310
[eclib-v20210310] /home/john/sage/build/bin/sage-dist-helpers: line 163: ./configure: No such file or directory
[eclib-v20210310] /home/john/sage/build/bin/sage-dist-helpers: line 165: ./configure: No such file or directory
[eclib-v20210310] ******************************************************************************************************************************************************
[eclib-v20210310] Error configuring eclib-v20210310

I know that the recent packaging changes are supposed to make the procedure easier, but it is only easy when you know how and the instructions are clearly not adequately telling *me* what to do. Help!

Last edited 14 months ago by cremona (previous) (diff)

comment:15 Changed 14 months ago by dimpase

You should use a configured tarball (created by make dist, typically. It would have ./configure script then ready), not the raw GitHub tarball, in upstream_url= field of checksums.ini.

Then ./sage -f should go through.

comment:16 Changed 14 months ago by dimpase

this patch makes it work:

  • build/pkgs/eclib/checksums.ini

    a b  
    1 tarball=eclib-20210310.tar.bz2
     1tarball=eclib-VERSION.tar.bz2
    22sha1=73437ac8deae94f00e7713405b3251d9c81f95e4
    33md5=4ac988bc46869866f076f7bea0fdaa6b
    44cksum=2130748042
    5 upstream_url=https://github.com/JohnCremona/eclib/archive/VERSION.tar.gz
     5upstream_url=https://johncremona.github.io/ftp/eclib-VERSION.tar.bz2

(needless to say, Sage's ./configure must be run with --enable-download-from-upstream-url for this tarball-downloading machinery to work)

comment:17 Changed 14 months ago by dimpase

Is this a non-backward compatible change in Sage's interface to eclib (many distros have versions of eclibs available, and they are used by default). If so, we also need to modify build/pkgs/eclib/spkg-configure.m4 to take this into account.

A quick fix would be to assume that pkg-config will now work with eclib, simpifying build/pkgs/eclib/spkg-configure.m4 a lot.

comment:18 Changed 14 months ago by cremona

I typed a long reply to that which just got zapped before I submitted the changes...

Yes, all previous versions of eclib will NOT work after these changes. So YES, we need to provide a method to check the version. The current m4 script cannot do that (the function version() it runs dates from before eclib had versin numbers and just displays compilation date and time). I can easily provide a function to return the version (which autotools defines as the macro VERSION). Perhaps more helpful would be a function to return the triple of integers (year, month, day), and also a function to compare the current version with a given (y,m,d) triple and return true iff the current version is equal or later than it. Then the little program constructed and run by the m4 could run (say) compare_eclib_version(2021, 3, 10) which would return true if and only if the actual version was equal or later than this. How does that sound?

In any case I can see that today will see a new version released with these functions in it, probably called 'v20210317'.

comment:19 follow-up: Changed 14 months ago by dimpase

pkg-config is standard, distros that don't use it consistently have only themselves to blame. With pkg-config, it's as simple as with fplll, say, see https://github.com/sagemath/sage/blob/develop/build/pkgs/fplll/spkg-configure.m4

Probably the separate check for mwrank should be kept, though. Can you add --version and/or -v option to mwrank, so that mwrank -v prints its version (make it equal to the eclib version) and exits?

Theoretically, there could be a situation that versions of mwrank and eclib are not the same on the system, so mwrank -v will allow to check for this consistency.

comment:20 in reply to: ↑ 19 Changed 14 months ago by dimpase

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

Replying to dimpase:

Theoretically, there could be a situation that versions of mwrank and eclib are not the same on the system, so mwrank -v will allow to check for this consistency.

copied this suggestion to https://github.com/JohnCremona/eclib/issues/67

comment:21 follow-up: Changed 14 months ago by cremona

That makes sense -- and I had mentioned this in the posting which disappeared. But I thought I had done this last time you (Dima) suggested it.

It makes little sense to talk about mwrank versions as different from eclib versions. mwrank is just the name of one of the executables built by eclib. But it does seem reasonable to use mwrank --version as a way for a system to tell which eclib version is installed. That is something different from your pkg-config suggestion though -- and eclib does have a file eclib.pc.

Now I will also comment on your eclib issue.

Last edited 14 months ago by slelievre (previous) (diff)

comment:22 in reply to: ↑ 21 Changed 14 months ago by dimpase

Replying to cremona:

That makes sense -- and I had mentioned this in the poting which disappeared. But I thought I had done this last time you (Dima) suggested it.

It makes little sense to talk about mwrank versions as different from eclib versions.

Say, one installs mwrank using an old eclib in /usr/local/bin, and has it shadow the new one mwrank in /usr/bin. We see such fun not quite bugs creeping up once in a while.

Comparing versions of eclib and mwrank is an extra check that won't hurt. pkg-config is useful in Python, as there is a Python interface to it (already used in Sage quite a bit to simplify the dread of getting the right libraries etc).

comment:23 follow-up: Changed 14 months ago by cremona

OK, so my current branch allows mwrank -V and outputs (e.g.) v20210317 and exits. None of my command-line options are multi-character and I am not going to change that now; as I said, '-v' is already taken.

So now we have pkg-config which reports the version, and also mwrank -V which does too.

All this still seems to be a lot of unnecessary both to me. Sage provides an mwrank executable via 'sage -mwrank' purely as a convenience to anyone out there (if there is anyone) who wants to install mwrank, as for such people I can say "install Sage and you'll get mwrank". That has been true for a decade or more, and now I can also say "use your distro package manager to install eclib, and you'll get mwrank". For historical reasons, Sage allows access to the mwrank binary in some of the elliptic curve functions, and also allows %mwrank to get a not very friendly user interface to mwrank (with no prompts). Both of these are really unnecessary and we could save some hassle by getting rid of them.

The point of what I am saying is that it matters not at all if anyone has a version of mwrank on their system which is not the same as the one provided by the eclib which Sage is using. A lot of the sage-eclib interface (including everything relevant to this ticket) is not about mwrank as such.

Can someone now explain to me how to change the m4 file to make use of the pkg-config thing? 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.

comment:24 Changed 14 months ago by git

  • Commit changed from f4752618381fc589d3b992d67cd29e9a9c7086d5 to 1bac7138020240bd3b6944e85bfc28676dcca6f1

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

1bac713more changes to eclib build setup

comment:25 Changed 14 months ago by cremona

  • Description modified (diff)

The latest version + updated tarball for eclib-v20210317 is closer. For me this works in that I could (re)install eclib using ./sage -f eclib (I know there is a new way to do that but this worked), where I had put a copy of the updated tarball into upstream so it did no downloading.

I made some changes to the m4 file, but I have not tested to see if that works, and it does not try to compare the version number with what mwrank -V gives out. I guess that I should at least install this eclib version on my system and then try to configure sage from scratch to see if it likes my system version. I would be very happy if someone else would like to improve the m4 file.

Last edited 14 months ago by slelievre (previous) (diff)

comment:26 Changed 14 months ago by dimpase

I'll look at the m4 file tonight.

comment:27 follow-up: Changed 14 months ago by cremona

The Sage configure system is mysterious to me. For example, I have flint-2.7.1 installed but the configure script outputs

flint-2.6.3:                                 no suitable system package; will be installed as an SPKG

I think it might be the case that as eclib lists ntl, flint and pari as prerequisites, it will build eclib if any of those three are also going to be built. That is why I was looking at the output for these. I was not surprised to see that it wanted to build NTL since my installed NTL was old -- I am now building ntl-11.4.4. (I am in the habit of installing eclib's dependencies by building from source.) Similarly with pari. Sage wants pari-2.11.4.p1 which I assume is 2.11.4 with some patching (why?). I have a 2.11.3 and 2.12.0.alpha but the last stable release is 2.13.1 so shoule Sage not upgrade?

Last edited 14 months ago by cremona (previous) (diff)

comment:28 in reply to: ↑ 27 Changed 14 months ago by dimpase

Replying to cremona:

The Sage configure system is mysterious to me. For example, I have flint-2.7.1 installed but the configure script outputs

flint-2.6.3:                                 no suitable system package; will be installed as an SPKG

I think it might be the case that as eclib lists ntl, flint and pari as prerequisites, it will build eclib if any of those three are also going to be built.

this is correct. Also, Sage is still on flint 2.6.3, see https://trac.sagemath.org/ticket/31069

more details can be found in the top-level config.log

That is why I was looking at the output for these. I was not surprised to see that it wanted to build NTL since my installed NTL was old -- I am now building ntl-11.4.4. (I am in the habit of installing eclib's dependencies by building from source.)

You might be willing to install them in /usr/local, then they will be picked up by ./configure, and (hopefully) used.

Similarly with pari. Sage wants pari-2.11.4.p1 which I assume is 2.11.4 with some patching (why?). I have a 2.11.3 and 2.12.0.alpha but the last stable release is 2.13.1 so shoule Sage not upgrade?

Pari upgrade to 2.13 is not yet done, see https://trac.sagemath.org/ticket/30906 - needs review.

As to what exactly is amiss in your current Pari, it can be seen in the top-level config log. There were several bugs in Pari 2.11 - were they all fixed in 2.11.4? I don't recall right away, there was a problematic Pari 2.12 release, perhaps something from there needed to be backported...

comment:29 Changed 14 months ago by cremona

Thanks for the detailed comments. I do install stuff in /usr/local, currently I have a problem with FLINT-2.7.2 whose "make check" now fails after I upgraded MPIR to 3.0.0 and MPFR to 4.0.0.

I will look at those two tickets, especially the pari one. Meanwhile I don't mind at all if Sage builds its own eclib, as in any case that will be essential now at least until there are any distros with my current version (which I have not yet tagged or released anyway).

comment:30 Changed 14 months ago by dimpase

the tarball URL is still wrong. (the one in checksums.ini)

Last edited 14 months ago by dimpase (previous) (diff)

comment:31 Changed 14 months ago by dimpase

there is also rubbish in the branch, a file named t.

comment:32 Changed 14 months ago by dimpase

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

comment:33 Changed 14 months ago by cremona

Sorry about that, but at least these are things I can easily fix...done, but trac is not allowing me to push the revised branch. I also updated the tarball at ​https://johncremona.github.io/ftp/eclib-v20210317.tar.bz2 (in case you had already downloaded it).

Last edited 14 months ago by cremona (previous) (diff)

comment:34 follow-up: Changed 14 months ago by cremona

Looking at my config.log, working backwards, I see that it wants to install eclib since it is installing ntl, it wants to install ntl (despite a new version being in /usr/local) because it is installing gmp/mpir, and it is installing gmp because it found no suitable system package, while for mpir it did find my new install and will not try to install that. But I *do* have gmp installed (libgmp-dev on ubuntu, package version 2:6.2.0+dsfg-4 on ubuntu 20.04). Dare I suggest that the configure script is incorrectly determining that gmp is not available?

comment:35 Changed 14 months ago by dimpase

by the way, why do you prepend v to to the version? This is an extra hassle. If it's just a number, or something like a.b.c, with a, b, c nonneg. integers, it's more stanard.

comment:36 in reply to: ↑ 34 Changed 14 months ago by dimpase

Replying to cremona:

Looking at my config.log, working backwards, I see that it wants to install eclib since it is installing ntl, it wants to install ntl (despite a new version being in /usr/local) because it is installing gmp/mpir, and it is installing gmp because it found no suitable system package, while for mpir it did find my new install and will not try to install that. But I *do* have gmp installed (libgmp-dev on ubuntu, package version 2:6.2.0+dsfg-4 on ubuntu 20.04). Dare I suggest that the configure script is incorrectly determining that gmp is not available?

Hmm, if you already have mpir/gmp installed in your Sage install, then ./configure will assume you want to keep it (and thus everything that depends upon it will be built).

make gmp-clean mpir-clean 

should clean these up, so that the system GMP is picked up by ./configure

comment:37 Changed 14 months ago by dimpase

  • Branch changed from u/cremona/31443-eclib-saturation to public/packages/eclib/31443
  • Commit changed from 1bac7138020240bd3b6944e85bfc28676dcca6f1 to 573bcf604b3ab69d19716b47627f3b378b0c081f

here are my updates, with fixed m4 etc.


New commits:

573bcf6fix m4 and tar URL, remove junk

comment:38 Changed 14 months ago by dimpase

I guess if the tarfile has changed then the checksums should be updated too.

comment:39 follow-up: Changed 14 months ago by cremona

I have nbot yet managed to push my branch, but I'll try again only after merging your fix for the mp4.

My version numbers did not use to have the v in them, I added that yesterday for some reason. I will revert that -- I think it was because my tag did have the v, so th tarball github creates has the v, an I thought I would be linking to that -- but we are not. This will require a fresh tarball, but that will not take long.

The configure tests I have been running are on a fresh download from trac, not an existing build, so *nothing* has been built in it yet! 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.

comment:40 Changed 14 months ago by slelievre

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

comment:41 Changed 14 months ago by cremona

  • Description modified (diff)

comment:42 in reply to: ↑ 39 ; follow-up: Changed 14 months ago by dimpase

  • Description modified (diff)

Replying to cremona:

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)

comment:43 Changed 14 months ago by cremona

  • Description modified (diff)

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?

comment:44 in reply to: ↑ 42 Changed 14 months ago by cremona

Replying to dimpase:

Replying to cremona:

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.

comment:45 follow-up: Changed 14 months ago by dimpase

do you by any chance use git trac extension?

comment:46 in reply to: ↑ 45 Changed 14 months ago by cremona

Replying to dimpase:

do you by any chance use git trac extension?

No

comment:47 follow-up: Changed 14 months ago by 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.

comment:48 in reply to: ↑ 47 Changed 14 months ago by cremona

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.

comment:49 follow-up: Changed 14 months ago by cremona

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.

comment:50 follow-up: Changed 14 months ago by dimpase

What does git remote -v say for this repo?

comment:51 in reply to: ↑ 49 ; follow-up: Changed 14 months ago by dimpase

Replying to cremona:

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

comment:52 in reply to: ↑ 50 ; follow-up: Changed 14 months ago by cremona

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!

comment:53 in reply to: ↑ 51 Changed 14 months ago by cremona

Replying to dimpase:

Replying to cremona:

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.

comment:54 in reply to: ↑ 52 Changed 14 months ago by dimpase

Replying to cremona:

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.

comment:55 Changed 14 months ago by git

  • Commit changed from 573bcf604b3ab69d19716b47627f3b378b0c081f to 926670fc33cc68d86bf002db4acb45c63f4df9ef

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

7db4910remove dud empty file
28408fccorrect upstream_url for eclib
d32295ffix spkg-configure for eclib
5a90432updated eclib build files to 20210318
926670fMerge remote-tracking branch 'jc/31443-eclib-saturation' into public/packages/eclib/31443

comment:56 follow-up: Changed 14 months ago by cremona

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.

comment:57 in reply to: ↑ 56 Changed 14 months ago by dimpase

Replying to cremona:

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)

comment:58 Changed 14 months ago by dimpase

  • Authors changed from John Cremona to John Cremona, Dima Pasechnik
  • Cc mjo isuruf added
  • Reviewers set to Dima Pasechnik

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

comment:59 in reply to: ↑ 23 ; follow-up: Changed 14 months ago by mkoeppe

  • Cc fbissey arojas added

Replying to cremona:

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.

comment:60 in reply to: ↑ 59 Changed 14 months ago by arojas

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.

comment:61 follow-ups: Changed 14 months ago by cremona

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.

comment:62 Changed 14 months ago by fbissey

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

comment:63 in reply to: ↑ 61 Changed 14 months ago by dimpase

Replying to cremona:

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.

comment:64 follow-up: Changed 14 months ago by cremona

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

comment:65 in reply to: ↑ 61 ; follow-up: Changed 14 months ago by mkoeppe

Replying to cremona:

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.

comment:66 in reply to: ↑ 65 Changed 14 months ago by cremona

Replying to mkoeppe:

Replying to cremona:

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.

comment:67 follow-up: Changed 14 months ago by 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.

comment:68 in reply to: ↑ 67 Changed 14 months ago by cremona

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.

comment:69 in reply to: ↑ 64 ; follow-up: Changed 14 months ago by mkoeppe

Replying to cremona:

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.

comment:70 Changed 14 months ago by git

  • Commit changed from 926670fc33cc68d86bf002db4acb45c63f4df9ef to 8b64fed5ba394314edc1e0200bb6e194a5fa00f2

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

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

comment:71 Changed 14 months ago by cremona

  • Description modified (diff)

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.

comment:72 in reply to: ↑ 69 Changed 14 months ago by cremona

Replying to mkoeppe:

Replying to cremona:

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.

Last edited 14 months ago by cremona (previous) (diff)

comment:73 Changed 14 months ago by mkoeppe

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, https://github.com/mkoeppe/sage/actions/runs/680630789

comment:74 Changed 14 months ago by mkoeppe

  • Reviewers changed from Dima Pasechnik, https://github.com/mkoeppe/sage/actions/runs/680630789 to Dima Pasechnik, Matthias Koeppe
  • Status changed from needs_review to positive_review

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

comment:75 Changed 14 months ago by cremona

Thanks to all for the reviewing and other help.

comment:76 Changed 14 months ago by cremona

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!

comment:77 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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

comment:78 Changed 14 months ago by cremona

I am about to update this with eclib version 20210408.

comment:79 Changed 14 months ago by git

  • Commit changed from 8b64fed5ba394314edc1e0200bb6e194a5fa00f2 to 8c4c115055b81b47860da66dbe0b0dde7bb55069
  • Status changed from positive_review to needs_review

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

17e91caMerge remote-tracking branch 'trac/develop' into 31443-eclib-saturation
f18fa63Merge remote-tracking branch 'trac/develop' into 31443-eclib-saturation
f564931updated eclib package version to 20210408 and checksums
07c5a20correct a docstring after eclib upgrade
8c4c115Merge branch 'public/packages/eclib/31443' of trac.sagemath.org:sage into 31443-eclib-saturation

comment:80 Changed 14 months ago by cremona

  • Description modified (diff)

Updated tarball to eclib-20210408. This has a bugfix but is otherwise compatible with the previous version. The onlt Sage files changed are the package version and checksums, and one docstring (which used to say that one example took 45 minutes, but now it only takes 40 seconds. In Sage-9.3 it will take many hours)

comment:81 follow-up: Changed 14 months ago by mjo

The version check in build/pkgs/eclib/spkg-configure.m4 needs to be updated to match the new version string.

I also spotted a typo "casue" in sage/libs/eclib/interface.py.

comment:82 in reply to: ↑ 81 Changed 14 months ago by cremona

Replying to mjo:

The version check in build/pkgs/eclib/spkg-configure.m4 needs to be updated to match the new version string.

I also spotted a typo "casue" in sage/libs/eclib/interface.py.

Thanks, I will fix both right away.

comment:83 Changed 14 months ago by git

  • Commit changed from 8c4c115055b81b47860da66dbe0b0dde7bb55069 to 9b1dedb02560d1b3fce444e5f88ac14f42786ce9

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

9b1dedbcorrect typo and update eclib's m4 script

comment:84 Changed 13 months ago by cremona

  • Description modified (diff)

Updated tarball to 20210415, version + checksums + m4 script. Some bugfixes compared with version 20210408. No more Sage code changes needed.

comment:85 Changed 13 months ago by git

  • Commit changed from 9b1dedb02560d1b3fce444e5f88ac14f42786ce9 to 3dca4e6c927053770c5e938f5a59be9047602cde

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

53a0305Merge remote-tracking branch 'trac/develop' into 31443-eclib-saturation
3dca4e6#31443: undated pkg info for 20210415

comment:86 Changed 13 months ago by git

  • Commit changed from 3dca4e6c927053770c5e938f5a59be9047602cde to 9443892b2e86d4edf4f040fa7b1736583ab4c29a

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

0ed01dcMerge branch 'develop' into 31443-eclib-saturation
9443892#31443: updated eclib tarball to 20210503

comment:87 Changed 13 months ago by cremona

  • Description modified (diff)

I am taking the opportunity of the delay to make a small additional update to the eclib tarball (release 20210503), with a minor bugfix. This version also keep debian happy on all its test platforms. No further Sage code changes outside build/pks/eclib.

comment:88 follow-up: Changed 12 months ago by dimpase

Is this tested with #30801 (upgrade to Pari 2.13.1) ?

comment:89 in reply to: ↑ 88 ; follow-up: Changed 12 months ago by arojas

Replying to dimpase:

Is this tested with #30801 (upgrade to Pari 2.13.1) ?

I've been shipping both downstream for a while without issues

comment:90 in reply to: ↑ 89 Changed 12 months ago by cremona

Replying to arojas:

Replying to dimpase:

Is this tested with #30801 (upgrade to Pari 2.13.1) ?

I've been shipping both downstream for a while without issues

I have been using recent pari version for eclib development, though eclib does not use very much of pari and nothing at all new.

comment:91 Changed 12 months ago by dimpase

  • Status changed from needs_review to positive_review

OK, fine.

comment:92 Changed 12 months ago by cremona

Is there a reason why this was not included in 9.4.beta0?

comment:93 Changed 12 months ago by slelievre

I'm pretty sure it's simply a matter of backlog as many tickets have had positive review for a while.

comment:94 Changed 12 months ago by vbraun

  • Status changed from positive_review to needs_work

PDF docs don't build

[docpdf] ! Undefined control sequence.
[docpdf] l.19194 ...q\) of good reduction such that \(E(\FF
[docpdf]                                                   _q)\) has
[docpdf] ? 
[docpdf] ! Emergency stop.

comment:95 follow-up: Changed 12 months ago by slelievre

Change \FF_q to \GF{q}:

-        modulo primes `q` of good reduction such that `E(\FF_q)` has
+        modulo primes `q` of good reduction such that `E(\GF{q})` has

comment:96 Changed 12 months ago by git

  • Commit changed from 9443892b2e86d4edf4f040fa7b1736583ab4c29a to b50d9c6047c1ad5167af7db8a8127f72ab18a853

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

d193588Merge tag '9.3' of github.com:sagemath/sage into public/packages/eclib/31443
b50d9c6Fix doc building

comment:97 in reply to: ↑ 95 Changed 12 months ago by cremona

Replying to slelievre:

Change \FF_q to \GF{q}:

-        modulo primes `q` of good reduction such that `E(\FF_q)` has
+        modulo primes `q` of good reduction such that `E(\GF{q})` has

Thanks for fixing this.

comment:98 Changed 12 months ago by slelievre

  • Status changed from needs_work to needs_review

Thanks Isuru for sending a commit implementing the suggested doc fix.

Back to needs review I guess. Or should it be rebased on 9.4.beta0?

comment:99 Changed 12 months ago by dimpase

  • Status changed from needs_review to positive_review

lgtm

comment:100 Changed 11 months ago by vbraun

  • Branch changed from public/packages/eclib/31443 to b50d9c6047c1ad5167af7db8a8127f72ab18a853
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:101 follow-up: Changed 11 months ago by vbraun

  • Commit b50d9c6047c1ad5167af7db8a8127f72ab18a853 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

On 32-bit:

**********************************************************************
File "src/sage/libs/eclib/interface.py", line 555, in sage.libs.eclib.interface.mwrank_EllipticCurve.saturate
Failed example:
    E.gens()
Expected:
    [[-1001107, -4004428, 1]]
Got:
    Attempt to convert -2004462486322 to long fails!
    [[-1001107, -4004428, 1]]
**********************************************************************
1 item had failures:
   1 of   9 in sage.libs.eclib.interface.mwrank_EllipticCurve.saturate
    [192 tests, 1 failure, 18.73 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/libs/eclib/interface.py  # 1 doctest failed
----------------------------------------------------------------------

comment:102 Changed 11 months ago by vbraun

  • Branch changed from b50d9c6047c1ad5167af7db8a8127f72ab18a853 to public/packages/eclib/31443
  • Commit set to b50d9c6047c1ad5167af7db8a8127f72ab18a853

Last 10 new commits:

926670fMerge remote-tracking branch 'jc/31443-eclib-saturation' into public/packages/eclib/31443
8b64fedMerge remote-tracking branch 'trac/public/packages/eclib/31443' into 31443-eclib-saturation
8c4c115Merge branch 'public/packages/eclib/31443' of trac.sagemath.org:sage into 31443-eclib-saturation
9b1dedbcorrect typo and update eclib's m4 script
53a0305Merge remote-tracking branch 'trac/develop' into 31443-eclib-saturation
3dca4e6#31443: undated pkg info for 20210415
0ed01dcMerge branch 'develop' into 31443-eclib-saturation
9443892#31443: updated eclib tarball to 20210503
d193588Merge tag '9.3' of github.com:sagemath/sage into public/packages/eclib/31443
b50d9c6Fix doc building

comment:103 in reply to: ↑ 101 Changed 11 months ago by cremona

Replying to vbraun:

On 32-bit:

**********************************************************************
File "src/sage/libs/eclib/interface.py", line 555, in sage.libs.eclib.interface.mwrank_EllipticCurve.saturate
Failed example:
    E.gens()
Expected:
    [[-1001107, -4004428, 1]]
Got:
    Attempt to convert -2004462486322 to long fails!
    [[-1001107, -4004428, 1]]
**********************************************************************
1 item had failures:
   1 of   9 in sage.libs.eclib.interface.mwrank_EllipticCurve.saturate
    [192 tests, 1 failure, 18.73 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/libs/eclib/interface.py  # 1 doctest failed
----------------------------------------------------------------------

Are we still supporting 32-bit? What if an upstream package does not support it? I have no way to test eclib on 32-bit.

comment:104 follow-up: Changed 11 months ago by dimpase

one can test on 32-bit using GitHub Actions (this would be very easy to set up for standalone eclib), and we still support 32-bit, see http://www.mirrorservice.org/sites/www.sagemath.org/linux/32bit/index.html

comment:105 in reply to: ↑ 104 Changed 11 months ago by cremona

Replying to dimpase:

one can test on 32-bit using GitHub Actions (this would be very easy to set up for standalone eclib), and we still support 32-bit, see http://www.mirrorservice.org/sites/www.sagemath.org/linux/32bit/index.html

Thanks, Dima. If you ever feel like helping me set up GitHub actions for eclib, that would be good. I have been using Travis.

comment:106 Changed 11 months ago by cremona

There is nothing special about the one example which triggers that error on 32-bit machines, and I will just change it to a smaller one. This version of eclib contains may improvements to the code for saturation so it was tempting to put in a non-trivial example in this doctest but it is not necessary.

comment:107 Changed 11 months ago by dimpase

how does one trigger it? perhaps put this into spkg-check?

comment:108 Changed 11 months ago by dimpase

there is also a way to make the test output dependent on the cpu, look up special tags 8n the code, # 32-bit if i recall right

comment:109 Changed 11 months ago by cremona

Yes I know about having 32-bit tests, when I started Sage development I had a 32-bit machine. But now I have not yet been able to build a 32-bit eclib (it is not enough to add -m32 to the compiler flags, it seems, and it might need a 32-bit version of the dependent libraries too).

The only purpose of this test is to show that saturation may not change anything. I'll patch it to use this smaller example instead:

sage: E = mwrank_EllipticCurve([0, 0, 0, -391, 0])                                                                                                                                                                           
sage: E.gens()                                                                                                                                                                                                               
[[196, 2730, 1]]
sage: E.saturate()                                                                                                                                                                                                           
sage: E.gens()                                                                                                                                                                                                               
[[196, 2730, 1]]

comment:110 Changed 11 months ago by dimpase

I've just built Sage 9.4.beta3 with this ticket on a 32-bit box, and checked that the following works:

  • src/sage/libs/eclib/interface.py

    a b class mwrank_EllipticCurve(SageObject): 
    553553
    554554            sage: E = mwrank_EllipticCurve([0, 0, 0, -1002231243161, 0])
    555555            sage: E.gens()
    556             [[-1001107, -4004428, 1]]
     556            [[-1001107, -4004428, 1]] # 64-bit
     557            ...                       # 32-bit
     558            [[-1001107, -4004428, 1]] # 32-bit
    557559            sage: E.saturate()
    558560            sage: E.gens()
    559561            [[-1001107, -4004428, 1]]

I can give you access to the box (well, it's old and slow) if you send me your public ssh key.

comment:111 Changed 11 months ago by git

  • Commit changed from b50d9c6047c1ad5167af7db8a8127f72ab18a853 to 7bed91d083c7a6b97b37a4af18579be9cedb80e2

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

7bed91dallow 32-bit boxes to complain

comment:112 Changed 11 months ago by dimpase

  • Status changed from new to needs_review

comment:113 Changed 11 months ago by dimpase

  • Status changed from needs_review to positive_review

comment:114 Changed 11 months ago by cremona

  • Status changed from positive_review to needs_work

I am tempted let this stand as is, but the warning line which 32-bit machines output could be fatal in that the code has attempted to convert a multiprecision integer to a long int (which is 64-bit or 32-bit, depending), and fails since 2004462486322 has 41 bits. The conversion function returns 0. So the output cannot be trusted, though here it does not give an incorrect result.

I have tracked down exactly where in eclib code this conversion happens. It is in a new piece of code in this version of eclib. It may occur when there are primes of bad reduction greater than (about) 231 (or 263). A better solution which I can do here is to use multiprecision integers in one place where I did not, so I will make that change and make a new eclib version.

Thanks anyway, Dima.

comment:115 Changed 11 months ago by cremona

  • Description modified (diff)

New commits:

7bed91dallow 32-bit boxes to complain

comment:116 Changed 11 months ago by cremona

I'll revert that cosmetic 32-bit fix since after fixing the underlying issue in eclib (new version 20210625) it will not be needed. New patch up soon.

comment:117 Changed 11 months ago by git

  • Commit changed from 7bed91d083c7a6b97b37a4af18579be9cedb80e2 to 789550ca04c94acfb1e803251538996a34962038

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

2c5b93bMerge branch 'develop' into eclib31443
c1871caMerge branch 'public/packages/eclib/31443' of trac.sagemath.org:sage into eclib31443
789550c31443 updated for eclib version 20210625

comment:118 Changed 11 months ago by cremona

  • Status changed from needs_work to needs_review

This version should pass OK on 32-bit and 64.

comment:119 follow-up: Changed 11 months ago by dimpase

  • Status changed from needs_review to positive_review

both on 32 and 64 bits I get this error:

sage -t --random-seed=0 src/sage/rings/number_field/number_field_ideal.py
**********************************************************************
File "src/sage/rings/number_field/number_field_ideal.py", line 2199, in sage.rings.number_field.number_field_ideal.NumberFieldFractionalIdeal.invertible_residues
Failed example:
    list(K.ideal(8).invertible_residues())[:5]
Expected:
    [1, a - 1, -3*a, -2*a + 3, -a - 1]
Got:
    [1, a + 2, 3*a + 3, -2*a + 3, a]

but this does not seem to be related to eclib. Positive review.

comment:120 follow-up: Changed 11 months ago by cremona

Thanks -- and you are right, that error cannot have anything to do with eclib. Though I wrote that function and test -- it uses pari. Probably a good idea to sort the result of this method (which just calls a more general one which returns an iterator).

comment:121 in reply to: ↑ 120 Changed 11 months ago by dimpase

Replying to cremona:

Thanks -- and you are right, that error cannot have anything to do with eclib. Though I wrote that function and test -- it uses pari. Probably a good idea to sort the result of this method (which just calls a more general one which returns an iterator).

it is not just sorting - the sets are not the same!

comment:122 Changed 11 months ago by cremona

A better test would be that the list contains 48 elements, namely x+y*a for x,y in range(8) not both even, up to congruence mod 8. Which is a bit messy, but writing such a test now would save having to go through all this again next time there's any change in pari. Say

sage: L = K.ideal(8).invertible_residues()                                                                                                                                       
sage: L1 = {(x%8,y%8) for x,y in L}                                                                                                                                              
sage: L2 = {(x,y) for x in range(8) for y in range(8) if (x%2,y%2) != (0,0)}                                                                                                     
sage: assert L1==L2 

or just

sage: {(x%8,y%8) for x,y in K.ideal(8).invertible_residues()} == {(x,y) for x in range(8) for y in range(8) if (x%2,y%2) != (0,0)}                                               
True

-- but on a separate ticket!

comment:123 Changed 11 months ago by vbraun

  • Branch changed from public/packages/eclib/31443 to 789550ca04c94acfb1e803251538996a34962038
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:124 Changed 11 months ago by mkoeppe

  • Commit 789550ca04c94acfb1e803251538996a34962038 deleted

This update appears to have broken ubuntu-trusty. https://github.com/sagemath/sage/runs/2967004002?check_suite_focus=true

/bin/bash ../libtool  --tag=CXX   --mode=compile g++ -std=gnu++11 -DPACKAGE_NAME=\"eclib\" -DPACKAGE_TARNAME=\"eclib\" -DPACKAGE_VERSION=\"20210625\" -DPACKAGE_STRING=\"eclib\ 20210625\" -DPACKAGE_BUGREPORT=\"john.cremona@gmail.com\" -DPACKAGE_URL=\"\" -DPACKAGE=\"eclib\" -DVERSION=\"20210625\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_ALLOCA_H=1 -DHAVE_ALLOCA=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_UNISTD_H=1 -DHAVE__BOOL=1 -DHAVE_STDBOOL_H=1 -Drestrict=__restrict -DHAVE_MEMMOVE=1 -DHAVE_MEMSET=1 -DHAVE_STRCHR=1 -I.  -DFLINT_LEVEL=1 -I/sage/local/include -I/sage/local/include -I/sage/local/include   -pthread   -O2 -g  -c -o isogs.lo isogs.cc
libtool: compile:  g++ -std=gnu++11 -DPACKAGE_NAME=\"eclib\" -DPACKAGE_TARNAME=\"eclib\" -DPACKAGE_VERSION=\"20210625\" "-DPACKAGE_STRING=\"eclib 20210625\"" -DPACKAGE_BUGREPORT=\"john.cremona@gmail.com\" -DPACKAGE_URL=\"\" -DPACKAGE=\"eclib\" -DVERSION=\"20210625\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_ALLOCA_H=1 -DHAVE_ALLOCA=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_UNISTD_H=1 -DHAVE__BOOL=1 -DHAVE_STDBOOL_H=1 -Drestrict=__restrict -DHAVE_MEMMOVE=1 -DHAVE_MEMSET=1 -DHAVE_STRCHR=1 -I. -DFLINT_LEVEL=1 -I/sage/local/include -I/sage/local/include -I/sage/local/include -pthread -O2 -g -c isogs.cc  -fPIC -DPIC -o .libs/isogs.o
In file included from /usr/include/c++/4.8/algorithm:62:0,
                 from ./eclib/interface.h:43,
                 from ./eclib/arith.h:27,
                 from ./eclib/marith.h:27,
                 from ./eclib/gf.h:29,
                 from ./eclib/polys.h:31,
                 from ./eclib/points.h:30,
                 from points.cc:26:
/usr/include/c++/4.8/bits/stl_algo.h: In instantiation of '_RandomAccessIterator std::__unguarded_partition(_RandomAccessIterator, _RandomAccessIterator, const _Tp&, _Compare) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<Point*, std::vector<Point> >; _Tp = Point; _Compare = Point_comparer]':
/usr/include/c++/4.8/bits/stl_algo.h:2296:78:   required from '_RandomAccessIterator std::__unguarded_partition_pivot(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<Point*, std::vector<Point> >; _Compare = Point_comparer]'
/usr/include/c++/4.8/bits/stl_algo.h:2337:62:   required from 'void std::__introsort_loop(_RandomAccessIterator, _RandomAccessIterator, _Size, _Compare) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<Point*, std::vector<Point> >; _Size = long int; _Compare = Point_comparer]'
/usr/include/c++/4.8/bits/stl_algo.h:5499:44:   required from 'void std::sort(_RAIter, _RAIter, _Compare) [with _RAIter = __gnu_cxx::__normal_iterator<Point*, std::vector<Point> >; _Compare = Point_comparer]'
points.cc:404:53:   required from here
/usr/include/c++/4.8/bits/stl_algo.h:2263:35: error: no match for call to '(Point_comparer) (Point&, const Point&)'
    while (__comp(*__first, __pivot))
                                   ^
points.cc:307:8: note: candidate is:
 struct Point_comparer {
        ^
points.cc:308:8: note: bool Point_comparer::operator()(Point&, Point&)
   bool operator()(Point& P, Point& Q)
        ^
points.cc:308:8: note:   no known conversion for argument 2 from 'const Point' to 'Point&'
In file included from /usr/include/c++/4.8/algorithm:62:0,
                 from ./eclib/interface.h:43,
                 from ./eclib/arith.h:27,
                 from ./eclib/marith.h:27,
                 from ./eclib/gf.h:29,
                 from ./eclib/polys.h:31,
                 from ./eclib/points.h:30,
                 from points.cc:26:
/usr/include/c++/4.8/bits/stl_algo.h:2266:34: error: no match for call to '(Point_comparer) (const Point&, Point&)'
    while (__comp(__pivot, *__last))
                                  ^
points.cc:307:8: note: candidate is:
 struct Point_comparer {
        ^
points.cc:308:8: note: bool Point_comparer::operator()(Point&, Point&)
   bool operator()(Point& P, Point& Q)
        ^
points.cc:308:8: note:   no known conversion for argument 1 from 'const Point' to 'Point&'
make[4]: *** [points.lo] Error 1

comment:125 Changed 11 months ago by mkoeppe

Looks like Point_comparer::operator()(Point&, Point&) needs some const-correctness

comment:126 Changed 11 months ago by cremona

I will look at that but I hope we will not have to reopen this ticket. The code has passed with both gcc and clang using github-actions.

The Point_comparer is used to sort lists of torsion points (finite order) and uses the point's order as the first comparator, but calling order(P) has a side-effect of caching the order in P if it was not already there, so cannot be called on a const point. Of course I could use copies within the comparer.

Would this change get around the issue?

diff --git a/libsrc/points.cc b/libsrc/points.cc
index 1c612f4..714b73d 100644
--- a/libsrc/points.cc
+++ b/libsrc/points.cc
@@ -305,8 +305,9 @@ vector<Point> points_from_x(Curvedata &E, const bigrational& x)
 // (4) y-coordinate
 
 struct Point_comparer {
-  bool operator()(Point& P, Point& Q)
+  bool operator()(const Point& P0, const Point& Q0)
   {
+    Point P(P0), Q(Q0); // take copies as args must be const but order() requires non-const
     int s = order(P)-order(Q);
     if(s) return (s<0); // true if P has smaller order
     bigint t = P.getZ()-Q.getZ();

"trusty" is ubuntu 14.04 I think....

comment:127 Changed 11 months ago by mkoeppe

Thanks, this patch fixes it. I have opened #32101 - I would just put this in as a patch.

Yes, ubuntu-trusty is ancient and we will likely drop support for it at some point (a recent discussion appeared in #32074).

comment:128 in reply to: ↑ 119 Changed 11 months ago by gh-mwageringel

Replying to dimpase:

both on 32 and 64 bits I get this error:

sage -t --random-seed=0 src/sage/rings/number_field/number_field_ideal.py
**********************************************************************
File "src/sage/rings/number_field/number_field_ideal.py", line 2199, in sage.rings.number_field.number_field_ideal.NumberFieldFractionalIdeal.invertible_residues
Failed example:
    list(K.ideal(8).invertible_residues())[:5]
Expected:
    [1, a - 1, -3*a, -2*a + 3, -a - 1]
Got:
    [1, a + 2, 3*a + 3, -2*a + 3, a]

This is now #32165.

Note: See TracTickets for help on using tickets.