#31443 closed enhancement (fixed)
update eclib and improve interface for elliptic curve saturation
Reported by:  cremona  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  packages: standard  Keywords:  eclib elliptic curve saturation 
Cc:  slelievre, ghcollares, 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: 
Description (last modified by )
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/eclib20210625.tar.bz2
Change History (128)
comment:1 Changed 15 months ago by
 Cc slelievre added
 Description modified (diff)
comment:2 Changed 15 months ago by
 Branch set to u/cremona/31443eclibsaturation
 Commit set to 69c3195d7adee86991ae3b9a47081f3222178d18
 Description modified (diff)
 Keywords eclib elliptic curve saturation added
 Status changed from new to needs_review
comment:3 Changed 15 months ago by
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
In fact the second example is even more stupid in 9.2 since after proving psaturation at all p up to 10979, with no errors or problems at all, it adds 1000 to the bound and starts again proving psaturation 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
 Cc ghcollares added
comment:7 Changed 14 months ago by
 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
 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 followup: ↓ 10 Changed 14 months ago by
checksums.ini
needs a new field upstream_url
, as explained in https://wiki.sagemath.org/ReleaseTours/sage9.1#For_developers1
comment:10 in reply to: ↑ 9 Changed 14 months ago by
Replying to mkoeppe:
checksums.ini
needs a new fieldupstream_url
, as explained in https://wiki.sagemath.org/ReleaseTours/sage9.1#For_developers1
Thanks, I will look at that tomorrow. I did reread 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
(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
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 fixchecksum eclib though the response was Checksum of eclib (tarball eclib20210310.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 distbzip2.
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 packageversion.txt? If that is the case I'll need to make another adjustment.
comment:13 Changed 14 months ago by
 Commit changed from 4ba267ac16722b3eee43d2b4ed73840c366a8bec to f4752618381fc589d3b992d67cd29e9a9c7086d5
Branch pushed to git repo; I updated commit sha1. New commits:
f475261  added upstream_url to eclib's checksums.ini

comment:14 Changed 14 months ago by
Now I also did what I thought I should have done, namely change packageversion.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=eclibVERSION.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 fixchecksum eclib
did change the checksums. I then did
./sage package update eclib v20210310
and then
./sage f eclib
but this failed quickly with
[eclibv20210310] Package 'eclib' is currently not installed [eclibv20210310] No legacy uninstaller found for 'eclib'; nothing to do [eclibv20210310] Deleting old versions of eclib libraries, which [eclibv20210310] would interfere with new builds... [eclibv20210310] Deleting old include directory... [eclibv20210310] withflint=/home/john/sage/local [eclibv20210310] Configuring eclibv20210310 [eclibv20210310] /home/john/sage/build/bin/sagedisthelpers: line 163: ./configure: No such file or directory [eclibv20210310] /home/john/sage/build/bin/sagedisthelpers: line 165: ./configure: No such file or directory [eclibv20210310] ****************************************************************************************************************************************************** [eclibv20210310] Error configuring eclibv20210310
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!
comment:15 Changed 14 months ago by
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
this patch makes it work:

build/pkgs/eclib/checksums.ini
a b 1 tarball=eclib 20210310.tar.bz21 tarball=eclibVERSION.tar.bz2 2 2 sha1=73437ac8deae94f00e7713405b3251d9c81f95e4 3 3 md5=4ac988bc46869866f076f7bea0fdaa6b 4 4 cksum=2130748042 5 upstream_url=https:// github.com/JohnCremona/eclib/archive/VERSION.tar.gz5 upstream_url=https://johncremona.github.io/ftp/eclibVERSION.tar.bz2
(needless to say, Sage's ./configure must be run with enabledownloadfromupstreamurl
for this tarballdownloading machinery to work)
comment:17 Changed 14 months ago by
Is this a nonbackward 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/spkgconfigure.m4
to take this into account.
A quick fix would be to assume that pkgconfig will now work with eclib, simpifying build/pkgs/eclib/spkgconfigure.m4 a lot.
comment:18 Changed 14 months ago by
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 followup: ↓ 20 Changed 14 months ago by
pkgconfig
is standard, distros that don't use it consistently have only themselves to blame.
With pkgconfig
, it's as simple as with fplll
, say, see
https://github.com/sagemath/sage/blob/develop/build/pkgs/fplll/spkgconfigure.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
 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 followup: ↓ 22 Changed 14 months ago by
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 pkgconfig suggestion though  and eclib does have a file eclib.pc
.
Now I will also comment on your eclib issue.
comment:22 in reply to: ↑ 21 Changed 14 months ago by
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. pkgconfig
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 followup: ↓ 59 Changed 14 months ago by
OK, so my current branch allows mwrank V
and outputs (e.g.) v20210317 and exits. None of my commandline options are multicharacter and I am not going to change that now; as I said, 'v' is already taken.
So now we have pkgconfig 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 sageeclib 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 pkgconfig 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 noone in the world (except me) who installs eclib except as part of Sage anyway.
comment:24 Changed 14 months ago by
 Commit changed from f4752618381fc589d3b992d67cd29e9a9c7086d5 to 1bac7138020240bd3b6944e85bfc28676dcca6f1
Branch pushed to git repo; I updated commit sha1. New commits:
1bac713  more changes to eclib build setup

comment:25 Changed 14 months ago by
 Description modified (diff)
The latest version + updated tarball for eclibv20210317 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.
comment:26 Changed 14 months ago by
I'll look at the m4 file tonight.
comment:27 followup: ↓ 28 Changed 14 months ago by
The Sage configure system is mysterious to me. For example, I have flint2.7.1 installed but the configure script outputs
flint2.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 ntl11.4.4. (I am in the habit of installing eclib's dependencies by building from source.) Similarly with pari. Sage wants pari2.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?
comment:28 in reply to: ↑ 27 Changed 14 months ago by
Replying to cremona:
The Sage configure system is mysterious to me. For example, I have flint2.7.1 installed but the configure script outputs
flint2.6.3: no suitable system package; will be installed as an SPKGI 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 toplevel 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 ntl11.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 pari2.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 toplevel 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
Thanks for the detailed comments. I do install stuff in /usr/local, currently I have a problem with FLINT2.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
the tarball URL is still wrong. (the one in checksums.ini)
comment:31 Changed 14 months ago by
there is also rubbish in the branch, a file named t
.
comment:32 Changed 14 months ago by
 Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.
comment:33 Changed 14 months ago by
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/eclibv20210317.tar.bz2 (in case you had already downloaded it).
comment:34 followup: ↓ 36 Changed 14 months ago by
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 (libgmpdev on ubuntu, package version 2:6.2.0+dsfg4 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
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
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 (libgmpdev on ubuntu, package version 2:6.2.0+dsfg4 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 gmpclean mpirclean
should clean these up, so that the system GMP is picked up by ./configure
comment:37 Changed 14 months ago by
 Branch changed from u/cremona/31443eclibsaturation to public/packages/eclib/31443
 Commit changed from 1bac7138020240bd3b6944e85bfc28676dcca6f1 to 573bcf604b3ab69d19716b47627f3b378b0c081f
comment:38 Changed 14 months ago by
I guess if the tarfile has changed then the checksums should be updated too.
comment:39 followup: ↓ 42 Changed 14 months ago by
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_64linuxgnu/libgmp.so.10 (0x00007f23ee737000)
so I don't know why Sage's configure does not find it. I do build mpir in its gmpcompat 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
Maybe ./configure withmp=gmp
can help?
comment:41 Changed 14 months ago by
 Description modified (diff)
comment:42 in reply to: ↑ 39 ; followup: ↓ 44 Changed 14 months ago by
 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_64linuxgnu/libgmp.so.10 (0x00007f23ee737000)so I don't know why Sage's configure does not find it. I do build mpir in its gmpcompat 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 toplevel config.log (This is a configuration noone tried since few years, I guess. MPIR is semiabandoned, I don't know why we have not downgraded it to experimental yet)
comment:43 Changed 14 months ago by
 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/31443eclibsaturation (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
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_64linuxgnu/libgmp.so.10 (0x00007f23ee737000)so I don't know why Sage's configure does not find it. I do build mpir in its gmpcompat 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 toplevel config.log (This is a configuration noone tried since few years, I guess. MPIR is semiabandoned, 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 followup: ↓ 46 Changed 14 months ago by
do you by any chance use git trac
extension?
comment:46 in reply to: ↑ 45 Changed 14 months ago by
comment:47 followup: ↓ 48 Changed 14 months ago by
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.12 (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
Replying to dimpase:
what do you get if you run
ssh git@trac.sagemath.org
? You'd see something likehello <username>, this is git@trac running gitolite3 3.5.3.12 (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.12 (Debian) on git 1.9.1 R W sage Connection to trac.sagemath.org closed.
which looks OK to me.
comment:49 followup: ↓ 51 Changed 14 months ago by
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 followup: ↓ 52 Changed 14 months ago by
What does git remote v
say for this repo?
comment:51 in reply to: ↑ 49 ; followup: ↓ 53 Changed 14 months ago by
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 ; followup: ↓ 54 Changed 14 months ago by
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 withmp=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 withmp=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
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 (noone 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 inbetween 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/31443eclibsaturation
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
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
 Commit changed from 573bcf604b3ab69d19716b47627f3b378b0c081f to 926670fc33cc68d86bf002db4acb45c63f4df9ef
Branch pushed to git repo; I updated commit sha1. New commits:
7db4910  remove dud empty file

28408fc  correct upstream_url for eclib

d32295f  fix spkgconfigure for eclib

5a90432  updated eclib build files to 20210318

926670f  Merge remotetracking branch 'jc/31443eclibsaturation' into public/packages/eclib/31443

comment:56 followup: ↓ 57 Changed 14 months ago by
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/31443eclibsaturation though that was made reduandant thanks to Dima's help.
comment:57 in reply to: ↑ 56 Changed 14 months ago by
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
 Cc mjo isuruf added
 Reviewers set to Dima Pasechnik
this looks good,perhaps someone who knowns autoconf can review my changes to spkgconfigure.m4
comment:59 in reply to: ↑ 23 ; followup: ↓ 60 Changed 14 months ago by
 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 noone 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
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 followups: ↓ 63 ↓ 65 Changed 14 months ago by
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
I can pin a version for sage9.2 and a different one for beta and future 9.3 in Gentoo.
comment:63 in reply to: ↑ 61 Changed 14 months ago by
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 followup: ↓ 69 Changed 14 months ago by
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 pointcounting 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 ; followup: ↓ 66 Changed 14 months ago by
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
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 followup: ↓ 68 Changed 14 months ago by
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
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 ; followup: ↓ 72 Changed 14 months ago by
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
 Commit changed from 926670fc33cc68d86bf002db4acb45c63f4df9ef to 8b64fed5ba394314edc1e0200bb6e194a5fa00f2
comment:71 Changed 14 months ago by
 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/eclibVERSION.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
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 withoutflint. 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 withoutflint the default, but there is no urgency.
comment:73 Changed 14 months ago by
 Reviewers changed from Dima Pasechnik to Dima Pasechnik, https://github.com/mkoeppe/sage/actions/runs/680630789
comment:74 Changed 14 months ago by
 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
Thanks to all for the reviewing and other help.
comment:76 Changed 14 months ago by
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
 Milestone changed from sage9.3 to sage9.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
I am about to update this with eclib version 20210408.
comment:79 Changed 14 months ago by
 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:
17e91ca  Merge remotetracking branch 'trac/develop' into 31443eclibsaturation

f18fa63  Merge remotetracking branch 'trac/develop' into 31443eclibsaturation

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 31443eclibsaturation

comment:80 Changed 14 months ago by
 Description modified (diff)
Updated tarball to eclib20210408. 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 Sage9.3 it will take many hours)
comment:81 followup: ↓ 82 Changed 14 months ago by
The version check in build/pkgs/eclib/spkgconfigure.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
Replying to mjo:
The version check in
build/pkgs/eclib/spkgconfigure.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
 Commit changed from 8c4c115055b81b47860da66dbe0b0dde7bb55069 to 9b1dedb02560d1b3fce444e5f88ac14f42786ce9
Branch pushed to git repo; I updated commit sha1. New commits:
9b1dedb  correct typo and update eclib's m4 script

comment:84 Changed 13 months ago by
 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
 Commit changed from 9b1dedb02560d1b3fce444e5f88ac14f42786ce9 to 3dca4e6c927053770c5e938f5a59be9047602cde
comment:86 Changed 13 months ago by
 Commit changed from 3dca4e6c927053770c5e938f5a59be9047602cde to 9443892b2e86d4edf4f040fa7b1736583ab4c29a
comment:87 Changed 13 months ago by
 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 followup: ↓ 89 Changed 12 months ago by
Is this tested with #30801 (upgrade to Pari 2.13.1) ?
comment:89 in reply to: ↑ 88 ; followup: ↓ 90 Changed 12 months ago by
comment:90 in reply to: ↑ 89 Changed 12 months ago by
comment:92 Changed 12 months ago by
Is there a reason why this was not included in 9.4.beta0?
comment:93 Changed 12 months ago by
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
 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 followup: ↓ 97 Changed 12 months ago by
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
 Commit changed from 9443892b2e86d4edf4f040fa7b1736583ab4c29a to b50d9c6047c1ad5167af7db8a8127f72ab18a853
comment:97 in reply to: ↑ 95 Changed 12 months ago by
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
 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:100 Changed 11 months ago by
 Branch changed from public/packages/eclib/31443 to b50d9c6047c1ad5167af7db8a8127f72ab18a853
 Resolution set to fixed
 Status changed from positive_review to closed
comment:101 followup: ↓ 103 Changed 11 months ago by
 Commit b50d9c6047c1ad5167af7db8a8127f72ab18a853 deleted
 Resolution fixed deleted
 Status changed from closed to new
On 32bit:
********************************************************************** 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 randomseed=0 src/sage/libs/eclib/interface.py # 1 doctest failed 
comment:102 Changed 11 months ago by
 Branch changed from b50d9c6047c1ad5167af7db8a8127f72ab18a853 to public/packages/eclib/31443
 Commit set to b50d9c6047c1ad5167af7db8a8127f72ab18a853
Last 10 new commits:
926670f  Merge remotetracking branch 'jc/31443eclibsaturation' into public/packages/eclib/31443

8b64fed  Merge remotetracking branch 'trac/public/packages/eclib/31443' into 31443eclibsaturation

8c4c115  Merge branch 'public/packages/eclib/31443' of trac.sagemath.org:sage into 31443eclibsaturation

9b1dedb  correct typo and update eclib's m4 script

53a0305  Merge remotetracking branch 'trac/develop' into 31443eclibsaturation

3dca4e6  #31443: undated pkg info for 20210415

0ed01dc  Merge branch 'develop' into 31443eclibsaturation

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

comment:103 in reply to: ↑ 101 Changed 11 months ago by
Replying to vbraun:
On 32bit:
********************************************************************** 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 randomseed=0 src/sage/libs/eclib/interface.py # 1 doctest failed 
Are we still supporting 32bit? What if an upstream package does not support it? I have no way to test eclib on 32bit.
comment:104 followup: ↓ 105 Changed 11 months ago by
one can test on 32bit using GitHub Actions (this would be very easy to set up for standalone eclib), and we still support 32bit, see http://www.mirrorservice.org/sites/www.sagemath.org/linux/32bit/index.html
comment:105 in reply to: ↑ 104 Changed 11 months ago by
Replying to dimpase:
one can test on 32bit using GitHub Actions (this would be very easy to set up for standalone eclib), and we still support 32bit, 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
There is nothing special about the one example which triggers that error on 32bit 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 nontrivial example in this doctest but it is not necessary.
comment:107 Changed 11 months ago by
how does one trigger it? perhaps put this into spkgcheck?
comment:108 Changed 11 months ago by
there is also a way to make the test output dependent on the cpu, look up special tags 8n the code, # 32bit
if i recall right
comment:109 Changed 11 months ago by
Yes I know about having 32bit tests, when I started Sage development I had a 32bit machine. But now I have not yet been able to build a 32bit eclib (it is not enough to add m32 to the compiler flags, it seems, and it might need a 32bit 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
I've just built Sage 9.4.beta3 with this ticket on a 32bit box, and checked that the following works:

src/sage/libs/eclib/interface.py
a b class mwrank_EllipticCurve(SageObject): 553 553 554 554 sage: E = mwrank_EllipticCurve([0, 0, 0, 1002231243161, 0]) 555 555 sage: E.gens() 556 [[1001107, 4004428, 1]] 556 [[1001107, 4004428, 1]] # 64bit 557 ... # 32bit 558 [[1001107, 4004428, 1]] # 32bit 557 559 sage: E.saturate() 558 560 sage: E.gens() 559 561 [[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
 Commit changed from b50d9c6047c1ad5167af7db8a8127f72ab18a853 to 7bed91d083c7a6b97b37a4af18579be9cedb80e2
Branch pushed to git repo; I updated commit sha1. New commits:
7bed91d  allow 32bit boxes to complain

comment:112 Changed 11 months ago by
 Status changed from new to needs_review
comment:113 Changed 11 months ago by
 Status changed from needs_review to positive_review
comment:114 Changed 11 months ago by
 Status changed from positive_review to needs_work
I am tempted let this stand as is, but the warning line which 32bit machines output could be fatal in that the code has attempted to convert a multiprecision integer to a long int (which is 64bit or 32bit, 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.
comment:115 Changed 11 months ago by
 Description modified (diff)
New commits:
7bed91d  allow 32bit boxes to complain

comment:116 Changed 11 months ago by
I'll revert that cosmetic 32bit 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
 Commit changed from 7bed91d083c7a6b97b37a4af18579be9cedb80e2 to 789550ca04c94acfb1e803251538996a34962038
comment:118 Changed 11 months ago by
 Status changed from needs_work to needs_review
This version should pass OK on 32bit and 64.
comment:119 followup: ↓ 128 Changed 11 months ago by
 Status changed from needs_review to positive_review
both on 32 and 64 bits I get this error:
sage t randomseed=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 followup: ↓ 121 Changed 11 months ago by
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
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
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
 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
 Commit 789550ca04c94acfb1e803251538996a34962038 deleted
This update appears to have broken ubuntutrusty
. 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
Looks like Point_comparer::operator()(Point&, Point&)
needs some constcorrectness
comment:126 Changed 11 months ago by
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 githubactions.
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 sideeffect 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) ycoordinate 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 nonconst 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
comment:128 in reply to: ↑ 119 Changed 11 months ago by
Replying to dimpase:
both on 32 and 64 bits I get this error:
sage t randomseed=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.
Here's the updated
eclib
versionv20210310
(see tarball link above) together with the necessary changes to the interface insrc/sage/libs/eclib
, and also a much simplified (and improved)saturation()
method insage/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 psaturation 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:
#31443: update eclib package metadata
#31443: update eclib package metadata again
#31443: adapt eclib library interface for version v20210310
#31443: simplify/improve saturation method for elliptic curves over Q