update eclib and improve interface for elliptic curve saturation

[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

[cremona]

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

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

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

[arojas]

Needs rebasing

[git]

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

[cremona]

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.

[mkoeppe]

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

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

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

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

[git]

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

​f475261

added upstream_url to eclib's checksums.ini

[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!

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

[dimpase]

this patch makes it work:

build/pkgs/eclib/checksums.ini

@@ -1 @@
-tarball=eclib-20210310.tar.bz2
+tarball=eclib-VERSION.tar.bz2
 sha1=73437ac8deae94f00e7713405b3251d9c81f95e4
 md5=4ac988bc46869866f076f7bea0fdaa6b
 cksum=2130748042
-upstream_url=https://github.com/JohnCremona/eclib/archive/VERSION.tar.gz
+upstream_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)

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

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

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

[dimpase]

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

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

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

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

[git]

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

​1bac713

more changes to eclib build setup

[cremona]

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.

[dimpase]

I'll look at the m4 file tonight.

[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?

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

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

[dimpase]

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

[dimpase]

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

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

[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?

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

[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

[dimpase]

here are my updates, with fixed m4 etc.

---

New commits:

​573bcf6

fix m4 and tar URL, remove junk

[dimpase]

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

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

[slelievre]

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

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

[cremona]

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?

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

[dimpase]

do you by any chance use git trac extension?

[cremona]

Replying to dimpase:

do you by any chance use git trac extension?

No

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

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

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

[dimpase]

What does git remote -v say for this repo?

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

[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!

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

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

[git]

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

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

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

[dimpase]

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

[mkoeppe]

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.

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

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

[fbissey]

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

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

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

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

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

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

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

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

[git]

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

[cremona]

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.

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

[mkoeppe]

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

[cremona]

Thanks to all for the reviewing and other help.

[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!

[mkoeppe]

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

[cremona]

I am about to update this with eclib version 20210408.

[git]

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

[cremona]

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)

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

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

[git]

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

​9b1dedb

correct typo and update eclib's m4 script

[cremona]

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

[git]

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

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

[git]

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

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

[cremona]

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.

[dimpase]

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

[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

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

[dimpase]

OK, fine.

[cremona]

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

[slelievre]

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

[vbraun]

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.

[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

[git]

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

​d193588	Merge tag '9.3' of github.com:sagemath/sage into public/packages/eclib/31443
​b50d9c6	Fix doc building

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

[slelievre]

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?

[dimpase]

lgtm

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

[vbraun]

Last 10 new commits:

​926670f	Merge remote-tracking branch 'jc/31443-eclib-saturation' into public/packages/eclib/31443
​8b64fed	Merge remote-tracking branch 'trac/public/packages/eclib/31443' into 31443-eclib-saturation
​8c4c115	Merge branch 'public/packages/eclib/31443' of trac.sagemath.org:sage into 31443-eclib-saturation
​9b1dedb	correct typo and update eclib's m4 script
​53a0305	Merge remote-tracking branch 'trac/develop' into 31443-eclib-saturation
​3dca4e6	#31443: undated pkg info for 20210415
​0ed01dc	Merge branch 'develop' into 31443-eclib-saturation
​9443892	#31443: updated eclib tarball to 20210503
​d193588	Merge tag '9.3' of github.com:sagemath/sage into public/packages/eclib/31443
​b50d9c6	Fix doc building

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

[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

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

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

[dimpase]

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

[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

[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]]

[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

@@ -553 +553 @@
 
             sage: E = mwrank_EllipticCurve([0, 0, 0, -1002231243161, 0])
             sage: E.gens()
-            [[-1001107, -4004428, 1]]
+            [[-1001107, -4004428, 1]] # 64-bit
+            ...                       # 32-bit
+            [[-1001107, -4004428, 1]] # 32-bit
             sage: E.saturate()
             sage: E.gens()
             [[-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.

[git]

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

​7bed91d

allow 32-bit boxes to complain

[cremona]

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) 2^{31 (or 2}63). 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.

[cremona]

New commits:

​7bed91d

allow 32-bit boxes to complain

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

[git]

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

​2c5b93b	Merge branch 'develop' into eclib31443
​c1871ca	Merge branch 'public/packages/eclib/31443' of trac.sagemath.org:sage into eclib31443
​789550c	31443 updated for eclib version 20210625

[cremona]

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

[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]

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

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

[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!

[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!

[mkoeppe]

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

[mkoeppe]

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

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

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

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