#31443 closed enhancement (fixed)
update eclib and improve interface for elliptic curve saturation
Reported by: | cremona | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.4 |
Component: | packages: standard | Keywords: | eclib elliptic curve saturation |
Cc: | slelievre, gh-collares, mjo, isuruf, fbissey, arojas | Merged in: | |
Authors: | John Cremona, Dima Pasechnik | Reviewers: | Dima Pasechnik, Matthias Koeppe |
Report Upstream: | Fixed upstream, in a later stable release. | Work issues: | |
Branch: | 789550c (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
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/eclib-20210625.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/31443-eclib-saturation
- 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 p-saturation at all p up to 10979, with no errors or problems at all, it adds 1000 to the bound and starts again proving p-saturation from 2 up to 11981. As each of these runs takes several minutes, it will take a long time to get up to 86682 this way. One issue (now addressed) is that eclib did not have a parameter specifying which p to start at, if you wanted to resume saturating at any point except 2 or 3 (there was only an option to skip p=2). But more seriously, the saturation was being repeated not because of any problem which has arisen, just because of the artificial lowering of the bound. The old version could do the job by setting max_prime
to 86682 but that takes a lot longer than the new version.
comment:5 Changed 15 months ago by
- Cc gh-collares 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 follow-up: ↓ 10 Changed 14 months ago by
checksums.ini
needs a new field upstream_url
, as explained in https://wiki.sagemath.org/ReleaseTours/sage-9.1#For_developers-1
comment:10 in reply to: ↑ 9 Changed 14 months ago by
Replying to mkoeppe:
checksums.ini
needs a new fieldupstream_url
, as explained in https://wiki.sagemath.org/ReleaseTours/sage-9.1#For_developers-1
Thanks, I will look at that tomorrow. I did re-read the packaging instructions to refresh my memory and found some new stuff, but the contents of checksums.ini is whatever was put there by whatever the instructions told me to do. (I have been updating the eclib package since about 2008...)
comment:11 Changed 14 months ago by
(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 fix-checksum eclib though the response was Checksum of eclib (tarball eclib-20210310.tar.bz2) unchanged
I'll update the branch in a minute, but I suspect that Sage will not like the fact that the upstream tarball (which is a tar.gz file created by github) is not the same as the tarball I put into upstream/ (and in the link on this ticket) which was created using make dist-bzip2.
If they need to be identical then I'll replace the actual tarball and then the checksum really will change.
The developers guide instructions are not at all clear. For example, I had assumed that one was suppose to replace the string 'VERSION' with the actual version number. Or is it supposed to be literally the string 'VERSION' which will be replaced by the content of the file package-version.txt? If that is the case I'll need to make another adjustment.
comment:13 Changed 14 months ago by
- 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 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!
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=eclib-VERSION.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/eclib-VERSION.tar.bz2
(needless to say, Sage's ./configure must be run with --enable-download-from-upstream-url
for this tarball-downloading machinery to work)
comment:17 Changed 14 months ago by
Is this a non-backward compatible change in Sage's interface to eclib (many distros have versions of eclibs available, and they are used by default).
If so, we also need to modify build/pkgs/eclib/spkg-configure.m4
to take this into account.
A quick fix would be to assume that pkg-config will now work with eclib, simpifying build/pkgs/eclib/spkg-configure.m4 a lot.
comment:18 Changed 14 months ago by
I typed a long reply to that which just got zapped before I submitted the changes...
Yes, all previous versions of eclib will NOT work after these changes. So YES, we need to provide a method to check the version. The current m4 script cannot do that (the function version() it runs dates from before eclib had versin numbers and just displays compilation date and time). I can easily provide a function to return the version (which autotools defines as the macro VERSION). Perhaps more helpful would be a function to return the triple of integers (year, month, day), and also a function to compare the current version with a given (y,m,d) triple and return true iff the current version is equal or later than it. Then the little program constructed and run by the m4 could run (say) compare_eclib_version(2021, 3, 10) which would return true if and only if the actual version was equal or later than this. How does that sound?
In any case I can see that today will see a new version released with these functions in it, probably called 'v20210317'.
comment:19 follow-up: ↓ 20 Changed 14 months ago by
pkg-config
is standard, distros that don't use it consistently have only themselves to blame.
With pkg-config
, it's as simple as with fplll
, say, see
https://github.com/sagemath/sage/blob/develop/build/pkgs/fplll/spkg-configure.m4
Probably the separate check for mwrank
should be kept, though. Can you add --version
and/or -v
option to mwrank
, so that mwrank -v
prints its version (make it equal to
the eclib
version) and exits?
Theoretically, there could be a situation that versions of mwrank and eclib are not the same on the system, so mwrank -v
will allow to check for this consistency.
comment:20 in reply to: ↑ 19 Changed 14 months ago by
- Report Upstream changed from N/A to Reported upstream. No feedback yet.
Replying to dimpase:
Theoretically, there could be a situation that versions of mwrank and eclib are not the same on the system, so
mwrank -v
will allow to check for this consistency.
copied this suggestion to https://github.com/JohnCremona/eclib/issues/67
comment:21 follow-up: ↓ 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 pkg-config 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. pkg-config
is useful in Python, as there is a Python interface to it (already used in Sage quite a bit
to simplify the dread of getting the right libraries etc).
comment:23 follow-up: ↓ 59 Changed 14 months ago by
OK, so my current branch allows mwrank -V
and outputs (e.g.) v20210317 and exits. None of my command-line options are multi-character and I am not going to change that now; as I said, '-v' is already taken.
So now we have pkg-config which reports the version, and also mwrank -V which does too.
All this still seems to be a lot of unnecessary both to me. Sage provides an mwrank executable via 'sage -mwrank' purely as a convenience to anyone out there (if there is anyone) who wants to install mwrank, as for such people I can say "install Sage and you'll get mwrank". That has been true for a decade or more, and now I can also say "use your distro package manager to install eclib, and you'll get mwrank". For historical reasons, Sage allows access to the mwrank binary in some of the elliptic curve functions, and also allows %mwrank to get a not very friendly user interface to mwrank (with no prompts). Both of these are really unnecessary and we could save some hassle by getting rid of them.
The point of what I am saying is that it matters not at all if anyone has a version of mwrank on their system which is not the same as the one provided by the eclib which Sage is using. A lot of the sage-eclib interface (including everything relevant to this ticket) is not about mwrank as such.
Can someone now explain to me how to change the m4 file to make use of the pkg-config thing? I think it would be quite reasonable to insist that the version installed on the system is *exactly* the version specified, not just "this or any later version", since I have enough to worry about without having to think about backwards compatibility. I'm also fairly sure that there is no-one in the world (except me) who installs eclib except as part of Sage anyway.
comment:24 Changed 14 months ago by
- 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 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.
comment:26 Changed 14 months ago by
I'll look at the m4 file tonight.
comment:27 follow-up: ↓ 28 Changed 14 months ago by
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?
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 flint-2.7.1 installed but the configure script outputs
flint-2.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 top-level config.log
That is why I was looking at the output for these. I was not surprised to see that it wanted to build NTL since my installed NTL was old -- I am now building ntl-11.4.4. (I am in the habit of installing eclib's dependencies by building from source.)
You might be willing to install them in /usr/local, then they will be picked up by ./configure, and (hopefully) used.
Similarly with pari. Sage wants pari-2.11.4.p1 which I assume is 2.11.4 with some patching (why?). I have a 2.11.3 and 2.12.0.alpha but the last stable release is 2.13.1 so shoule Sage not upgrade?
Pari upgrade to 2.13 is not yet done, see https://trac.sagemath.org/ticket/30906 - needs review.
As to what exactly is amiss in your current Pari, it can be seen in the top-level config log. There were several bugs in Pari 2.11 - were they all fixed in 2.11.4? I don't recall right away, there was a problematic Pari 2.12 release, perhaps something from there needed to be backported...
comment:29 Changed 14 months ago by
Thanks for the detailed comments. I do install stuff in /usr/local, currently I have a problem with FLINT-2.7.2 whose "make check" now fails after I upgraded MPIR to 3.0.0 and MPFR to 4.0.0.
I will look at those two tickets, especially the pari one. Meanwhile I don't mind at all if Sage builds its own eclib, as in any case that will be essential now at least until there are any distros with my current version (which I have not yet tagged or released anyway).
comment:30 Changed 14 months ago by
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/eclib-v20210317.tar.bz2 (in case you had already downloaded it).
comment:34 follow-up: ↓ 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 (libgmp-dev on ubuntu, package version 2:6.2.0+dsfg-4 on ubuntu 20.04). Dare I suggest that the configure script is incorrectly determining that gmp is not available?
comment:35 Changed 14 months ago by
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 (libgmp-dev on ubuntu, package version 2:6.2.0+dsfg-4 on ubuntu 20.04). Dare I suggest that the configure script is incorrectly determining that gmp is not available?
Hmm, if you already have mpir/gmp installed in your Sage install, then ./configure will assume you want to keep it (and thus everything that depends upon it will be built).
make gmp-clean mpir-clean
should clean these up, so that the system GMP is picked up by ./configure
comment:37 Changed 14 months ago by
- Branch changed from u/cremona/31443-eclib-saturation 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 follow-up: ↓ 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_64-linux-gnu/libgmp.so.10 (0x00007f23ee737000)
so I don't know why Sage's configure does not find it. I do build mpir in its gmp-compat mode, so as well as the system library as above, in /usr/local/lib I have both libmpir and libgmp both installed when I do 'make install' on mpir.
comment:40 Changed 14 months ago by
Maybe ./configure --with-mp=gmp
can help?
comment:41 Changed 14 months ago by
- Description modified (diff)
comment:42 in reply to: ↑ 39 ; follow-up: ↓ 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_64-linux-gnu/libgmp.so.10 (0x00007f23ee737000)so I don't know why Sage's configure does not find it. I do build mpir in its gmp-compat mode, so as well as the system library as above, in /usr/local/lib I have both libmpir and libgmp both installed when I do 'make install' on mpir.
Sage most probably finds mpir in /usr/local first, and then rejects it for some reason. Please post top-level config.log (This is a configuration noone tried since few years, I guess. MPIR is semi-abandoned, I don't know why we have not downgraded it to experimental yet)
comment:43 Changed 14 months ago by
- Description modified (diff)
I wil try that configure suggestion, thanks.
Meanwhile I have updated the branch with yet another version of the tarball, checksums etc, 20210318 (no v) but for some reason I cannot now push to trac, either to u/cremona/31443-eclib-saturation (by old branch) or public/packages/eclib/31443. In both cases I see
fatal: remote error: access denied or repository not exported: /sage.git
I have changed nothing with my ssh setup (no new keys, etc). Suggestions?
comment:44 in reply to: ↑ 42 Changed 14 months ago by
Replying to dimpase:
Replying to cremona:
eclib itself detects gmp, and when I run ldd on my latest version of the shared libec library I see
libgmp.so.10 => /usr/lib/x86_64-linux-gnu/libgmp.so.10 (0x00007f23ee737000)so I don't know why Sage's configure does not find it. I do build mpir in its gmp-compat mode, so as well as the system library as above, in /usr/local/lib I have both libmpir and libgmp both installed when I do 'make install' on mpir.
Sage most probably finds mpir in /usr/local first, and then rejects it for some reason. Please post top-level config.log (This is a configuration noone tried since few years, I guess. MPIR is semi-abandoned, I don't know why we have not downgraded it to experimental yet)
I am not tied at all to MPIR, I started using it when Bill Hart was in Warwick. But it will simplify m life a bit to just use ordinary gmp, so I will uninstall mpir and hope that this will fix things.
comment:45 follow-up: ↓ 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 follow-up: ↓ 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.1-2 (Debian) on git 1.9.1 R W sage Connection to trac.sagemath.org closed.
comment:48 in reply to: ↑ 47 Changed 14 months ago by
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.1-2 (Debian) on git 1.9.1 R W sage Connection to trac.sagemath.org closed.
I get this:
$ ssh git@trac.sagemath.org X11 forwarding request failed on channel 0 PTY allocation request failed on channel 0 hello cremona, this is git@trac running gitolite3 3.5.3.1-2 (Debian) on git 1.9.1 R W sage Connection to trac.sagemath.org closed.
which looks OK to me.
comment:49 follow-up: ↓ 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 follow-up: ↓ 52 Changed 14 months ago by
What does git remote -v
say for this repo?
comment:51 in reply to: ↑ 49 ; follow-up: ↓ 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 ; follow-up: ↓ 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 --with-mp=pir on the configure command line. That is progress. But it still does not like my NTL. Here's a bit of config.log:
## ----------------------------------------------------- ## ## Checking whether SageMath should install SPKG mpir... ## ## ----------------------------------------------------- ## configure:10351: no suitable system package found for SPKG mpir configure:10371: result: using mpir SPKG (via --with-mp=mpir) ## ---------------------------------------------------- ## ## Checking whether SageMath should install SPKG gmp... ## ## ---------------------------------------------------- ## configure:10526: will use system package and not install SPKG gmp ## ----------------------------------------------------- ## ## Checking whether SageMath should install SPKG mpfr... ## ## ----------------------------------------------------- ## configure:10624: checking installing gmp/mpir? configure:10627: result: yes; install mpfr as well configure:10715: no suitable system package found for SPKG mpfr configure:10733: result: using Sage's mpfr SPKG ## ---------------------------------------------------- ## ## Checking whether SageMath should install SPKG ntl... ## ## ---------------------------------------------------- ## configure:10814: checking installing gmp/mpir? configure:10817: result: yes; install ntl as well configure:10929: no suitable system package found for SPKG ntl ## ------------------------------------------------------ ## ## Checking whether SageMath should install SPKG flint... ## ## ------------------------------------------------------ ## configure:11136: checking installing mpfr or ntl? configure:11139: result: yes; install flint as well configure:11324: no suitable system package found for SPKG flint configure:11342: result: using Sage's flint SPKG ## ---------------------------------------------------- ## ## Checking whether SageMath should install SPKG arb... ## ## ---------------------------------------------------- ## configure:11424: checking installing flint? configure:11427: result: yes; install arb as well configure:11544: no suitable system package found for SPKG arb
This is so inconsistent!
comment:53 in reply to: ↑ 51 Changed 14 months ago by
Replying to dimpase:
Replying to cremona:
When I uninstalled MPIR I managed to so seriously break my system that gcc did not work. I even reinstalled ubuntu's gmp and gcc but they still did not work. So I reinstalled MPIR, which puts libgmp files (as well as libmpir files) into /usr/local, and now gcc works. Bizarre.
gmp is a build dependency of gcc, so not too bizzare.
I think it *is* bizarre since I reinstalled the system gmp and gcc and after that gcc did not work, while after reinstalling my own mpir build gcc does work.
However, system packages should not depend on anything in /usr/local, so perhaps something was not properly flushed...
I don't know exactly what that means or how to fix it.
The only reason I am in this mess is that I was trying to improve Sage for Sage users by improving eclib (no-one else uses it). I have now spent several days on all this package installation nonsense, which used to be very simple but is now in some in-between state where things only half work. And now I cannot even push my branch to trac for some other stupid reason. I did push it to my github fork https://github.com/JohnCremona/sage/tree/31443-eclib-saturation
but I have had enough of all this. I may come back to it next week, but I have others things to do.
comment:54 in reply to: ↑ 52 Changed 14 months ago by
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 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
|
comment:56 follow-up: ↓ 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/31443-eclib-saturation 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 spkg-configure.m4
comment:59 in reply to: ↑ 23 ; follow-up: ↓ 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 no-one in the world (except me) who installs eclib except as part of Sage anyway.
There is also a possible middle ground - you could set an upper bound as well such as <= 20231231
.
Does the new version of eclib
work with old version of Sage?
Downstream distributors of sage could perhaps comment whether fixing the eclib version creates additional burden for them.
comment:60 in reply to: ↑ 59 Changed 14 months ago by
Replying to mkoeppe:
Downstream distributors of sage could perhaps comment whether fixing the eclib version creates additional burden for them.
I don't use sage's build system, so personally I don't care which eclib version the m4 file expects. I always make sure that the versions of eclib and sagelib in our repos are compatible.
comment:61 follow-ups: ↓ 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 sage-9.2 and a different one for beta and future 9.3 in Gentoo.
comment:63 in reply to: ↑ 61 Changed 14 months ago by
Replying to cremona:
No, the new version of eclib will not work with older sages: the sage code changes here in the eclib interface are necesary.
I just push a tag '20210318' to eclib on github and will make a release with the same name.
From what I have been discovering about dependencies in Sage's own build system, since Sage has a patched version of pari it will always build any package with pari as a dependency.
no, this is not correct. There are versions of Pari 2.11 around on some Linux distributions which are accepted by Sage and may be used. Sage's patches to Pari are needed for Cygwin.
comment:64 follow-up: ↓ 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 point-counting on elliptic curves mod p. I could make that optional too since eclib can do its own point counting (it is just slower).
comment:65 in reply to: ↑ 61 ; follow-up: ↓ 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 follow-up: ↓ 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 ; follow-up: ↓ 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/eclib-VERSION.tar.bz2
with "VERSION" twice since the version string "20210318" appears twice in the URL. I hope that Sage can handle that.
comment:72 in reply to: ↑ 69 Changed 14 months ago by
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.
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 sage-9.3 to sage-9.4
Moving this ticket to 9.4, as it seems unlikely that it will be merged in 9.3, which is in the release candidate stage
comment:78 Changed 14 months ago by
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 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
|
comment:80 Changed 14 months ago by
- Description modified (diff)
Updated tarball to eclib-20210408. This has a bugfix but is otherwise compatible with the previous version. The onlt Sage files changed are the package version and checksums, and one docstring (which used to say that one example took 45 minutes, but now it only takes 40 seconds. In Sage-9.3 it will take many hours)
comment:81 follow-up: ↓ 82 Changed 14 months ago by
The version check in build/pkgs/eclib/spkg-configure.m4
needs to be updated to match the new version string.
I also spotted a typo "casue" in sage/libs/eclib/interface.py
.
comment:82 in reply to: ↑ 81 Changed 14 months ago by
Replying to mjo:
The version check in
build/pkgs/eclib/spkg-configure.m4
needs to be updated to match the new version string.I also spotted a typo "casue" in
sage/libs/eclib/interface.py
.
Thanks, I will fix both right away.
comment:83 Changed 14 months ago by
- 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 follow-up: ↓ 89 Changed 12 months ago by
Is this tested with #30801 (upgrade to Pari 2.13.1) ?
comment:89 in reply to: ↑ 88 ; follow-up: ↓ 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 follow-up: ↓ 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 follow-up: ↓ 103 Changed 11 months ago by
- Commit b50d9c6047c1ad5167af7db8a8127f72ab18a853 deleted
- Resolution fixed deleted
- Status changed from closed to new
On 32-bit:
********************************************************************** File "src/sage/libs/eclib/interface.py", line 555, in sage.libs.eclib.interface.mwrank_EllipticCurve.saturate Failed example: E.gens() Expected: [[-1001107, -4004428, 1]] Got: Attempt to convert -2004462486322 to long fails! [[-1001107, -4004428, 1]] ********************************************************************** 1 item had failures: 1 of 9 in sage.libs.eclib.interface.mwrank_EllipticCurve.saturate [192 tests, 1 failure, 18.73 s] ---------------------------------------------------------------------- sage -t --long --random-seed=0 src/sage/libs/eclib/interface.py # 1 doctest failed ----------------------------------------------------------------------
comment:102 Changed 11 months ago by
- Branch changed from b50d9c6047c1ad5167af7db8a8127f72ab18a853 to public/packages/eclib/31443
- Commit set to b50d9c6047c1ad5167af7db8a8127f72ab18a853
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
|
comment:103 in reply to: ↑ 101 Changed 11 months ago by
Replying to vbraun:
On 32-bit:
********************************************************************** File "src/sage/libs/eclib/interface.py", line 555, in sage.libs.eclib.interface.mwrank_EllipticCurve.saturate Failed example: E.gens() Expected: [[-1001107, -4004428, 1]] Got: Attempt to convert -2004462486322 to long fails! [[-1001107, -4004428, 1]] ********************************************************************** 1 item had failures: 1 of 9 in sage.libs.eclib.interface.mwrank_EllipticCurve.saturate [192 tests, 1 failure, 18.73 s] ---------------------------------------------------------------------- sage -t --long --random-seed=0 src/sage/libs/eclib/interface.py # 1 doctest failed ----------------------------------------------------------------------
Are we still supporting 32-bit? What if an upstream package does not support it? I have no way to test eclib on 32-bit.
comment:104 follow-up: ↓ 105 Changed 11 months ago by
one can test on 32-bit using GitHub Actions (this would be very easy to set up for standalone eclib), and we still support 32-bit, see http://www.mirrorservice.org/sites/www.sagemath.org/linux/32bit/index.html
comment:105 in reply to: ↑ 104 Changed 11 months ago by
Replying to dimpase:
one can test on 32-bit using GitHub Actions (this would be very easy to set up for standalone eclib), and we still support 32-bit, see http://www.mirrorservice.org/sites/www.sagemath.org/linux/32bit/index.html
Thanks, Dima. If you ever feel like helping me set up GitHub actions for eclib, that would be good. I have been using Travis.
comment:106 Changed 11 months ago by
There is nothing special about the one example which triggers that error on 32-bit machines, and I will just change it to a smaller one. This version of eclib contains may improvements to the code for saturation so it was tempting to put in a non-trivial example in this doctest but it is not necessary.
comment:107 Changed 11 months ago by
how does one trigger it? perhaps put this into spkg-check?
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, # 32-bit
if i recall right
comment:109 Changed 11 months ago by
Yes I know about having 32-bit tests, when I started Sage development I had a 32-bit machine. But now I have not yet been able to build a 32-bit eclib (it is not enough to add -m32 to the compiler flags, it seems, and it might need a 32-bit version of the dependent libraries too).
The only purpose of this test is to show that saturation may not change anything. I'll patch it to use this smaller example instead:
sage: E = mwrank_EllipticCurve([0, 0, 0, -391, 0]) sage: E.gens() [[196, 2730, 1]] sage: E.saturate() sage: E.gens() [[196, 2730, 1]]
comment:110 Changed 11 months ago by
I've just built Sage 9.4.beta3 with this ticket on a 32-bit box, and checked that the following works:
-
src/sage/libs/eclib/interface.py
a b class mwrank_EllipticCurve(SageObject): 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]] # 64-bit 557 ... # 32-bit 558 [[-1001107, -4004428, 1]] # 32-bit 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 32-bit 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 32-bit machines output could be fatal in that the code has attempted to convert a multiprecision integer to a long int (which is 64-bit or 32-bit, depending), and fails since 2004462486322 has 41 bits. The conversion function returns 0. So the output cannot be trusted, though here it does not give an incorrect result.
I have tracked down exactly where in eclib code this conversion happens. It is in a new piece of code in this version of eclib. It may occur when there are primes of bad reduction greater than (about) 231 (or 263). A better solution which I can do here is to use multiprecision integers in one place where I did not, so I will make that change and make a new eclib version.
Thanks anyway, Dima.
comment:115 Changed 11 months ago by
- Description modified (diff)
New commits:
7bed91d | allow 32-bit boxes to complain
|
comment:116 Changed 11 months ago by
I'll revert that cosmetic 32-bit fix since after fixing the underlying issue in eclib (new version 20210625) it will not be needed. New patch up soon.
comment:117 Changed 11 months ago by
- 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 32-bit and 64.
comment:119 follow-up: ↓ 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 --random-seed=0 src/sage/rings/number_field/number_field_ideal.py ********************************************************************** File "src/sage/rings/number_field/number_field_ideal.py", line 2199, in sage.rings.number_field.number_field_ideal.NumberFieldFractionalIdeal.invertible_residues Failed example: list(K.ideal(8).invertible_residues())[:5] Expected: [1, a - 1, -3*a, -2*a + 3, -a - 1] Got: [1, a + 2, 3*a + 3, -2*a + 3, a]
but this does not seem to be related to eclib. Positive review.
comment:120 follow-up: ↓ 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 ubuntu-trusty
. https://github.com/sagemath/sage/runs/2967004002?check_suite_focus=true
/bin/bash ../libtool --tag=CXX --mode=compile g++ -std=gnu++11 -DPACKAGE_NAME=\"eclib\" -DPACKAGE_TARNAME=\"eclib\" -DPACKAGE_VERSION=\"20210625\" -DPACKAGE_STRING=\"eclib\ 20210625\" -DPACKAGE_BUGREPORT=\"john.cremona@gmail.com\" -DPACKAGE_URL=\"\" -DPACKAGE=\"eclib\" -DVERSION=\"20210625\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_ALLOCA_H=1 -DHAVE_ALLOCA=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_UNISTD_H=1 -DHAVE__BOOL=1 -DHAVE_STDBOOL_H=1 -Drestrict=__restrict -DHAVE_MEMMOVE=1 -DHAVE_MEMSET=1 -DHAVE_STRCHR=1 -I. -DFLINT_LEVEL=1 -I/sage/local/include -I/sage/local/include -I/sage/local/include -pthread -O2 -g -c -o isogs.lo isogs.cc libtool: compile: g++ -std=gnu++11 -DPACKAGE_NAME=\"eclib\" -DPACKAGE_TARNAME=\"eclib\" -DPACKAGE_VERSION=\"20210625\" "-DPACKAGE_STRING=\"eclib 20210625\"" -DPACKAGE_BUGREPORT=\"john.cremona@gmail.com\" -DPACKAGE_URL=\"\" -DPACKAGE=\"eclib\" -DVERSION=\"20210625\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_ALLOCA_H=1 -DHAVE_ALLOCA=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_UNISTD_H=1 -DHAVE__BOOL=1 -DHAVE_STDBOOL_H=1 -Drestrict=__restrict -DHAVE_MEMMOVE=1 -DHAVE_MEMSET=1 -DHAVE_STRCHR=1 -I. -DFLINT_LEVEL=1 -I/sage/local/include -I/sage/local/include -I/sage/local/include -pthread -O2 -g -c isogs.cc -fPIC -DPIC -o .libs/isogs.o In file included from /usr/include/c++/4.8/algorithm:62:0, from ./eclib/interface.h:43, from ./eclib/arith.h:27, from ./eclib/marith.h:27, from ./eclib/gf.h:29, from ./eclib/polys.h:31, from ./eclib/points.h:30, from points.cc:26: /usr/include/c++/4.8/bits/stl_algo.h: In instantiation of '_RandomAccessIterator std::__unguarded_partition(_RandomAccessIterator, _RandomAccessIterator, const _Tp&, _Compare) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<Point*, std::vector<Point> >; _Tp = Point; _Compare = Point_comparer]': /usr/include/c++/4.8/bits/stl_algo.h:2296:78: required from '_RandomAccessIterator std::__unguarded_partition_pivot(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<Point*, std::vector<Point> >; _Compare = Point_comparer]' /usr/include/c++/4.8/bits/stl_algo.h:2337:62: required from 'void std::__introsort_loop(_RandomAccessIterator, _RandomAccessIterator, _Size, _Compare) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<Point*, std::vector<Point> >; _Size = long int; _Compare = Point_comparer]' /usr/include/c++/4.8/bits/stl_algo.h:5499:44: required from 'void std::sort(_RAIter, _RAIter, _Compare) [with _RAIter = __gnu_cxx::__normal_iterator<Point*, std::vector<Point> >; _Compare = Point_comparer]' points.cc:404:53: required from here /usr/include/c++/4.8/bits/stl_algo.h:2263:35: error: no match for call to '(Point_comparer) (Point&, const Point&)' while (__comp(*__first, __pivot)) ^ points.cc:307:8: note: candidate is: struct Point_comparer { ^ points.cc:308:8: note: bool Point_comparer::operator()(Point&, Point&) bool operator()(Point& P, Point& Q) ^ points.cc:308:8: note: no known conversion for argument 2 from 'const Point' to 'Point&' In file included from /usr/include/c++/4.8/algorithm:62:0, from ./eclib/interface.h:43, from ./eclib/arith.h:27, from ./eclib/marith.h:27, from ./eclib/gf.h:29, from ./eclib/polys.h:31, from ./eclib/points.h:30, from points.cc:26: /usr/include/c++/4.8/bits/stl_algo.h:2266:34: error: no match for call to '(Point_comparer) (const Point&, Point&)' while (__comp(__pivot, *__last)) ^ points.cc:307:8: note: candidate is: struct Point_comparer { ^ points.cc:308:8: note: bool Point_comparer::operator()(Point&, Point&) bool operator()(Point& P, Point& Q) ^ points.cc:308:8: note: no known conversion for argument 1 from 'const Point' to 'Point&' make[4]: *** [points.lo] Error 1
comment:125 Changed 11 months ago by
Looks like Point_comparer::operator()(Point&, Point&)
needs some const-correctness
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 github-actions.
The Point_comparer is used to sort lists of torsion points (finite order) and uses the point's order as the first comparator, but calling order(P) has a side-effect of caching the order in P if it was not already there, so cannot be called on a const point. Of course I could use copies within the comparer.
Would this change get around the issue?
diff --git a/libsrc/points.cc b/libsrc/points.cc index 1c612f4..714b73d 100644 --- a/libsrc/points.cc +++ b/libsrc/points.cc @@ -305,8 +305,9 @@ vector<Point> points_from_x(Curvedata &E, const bigrational& x) // (4) y-coordinate struct Point_comparer { - bool operator()(Point& P, Point& Q) + bool operator()(const Point& P0, const Point& Q0) { + Point P(P0), Q(Q0); // take copies as args must be const but order() requires non-const int s = order(P)-order(Q); if(s) return (s<0); // true if P has smaller order bigint t = P.getZ()-Q.getZ();
"trusty" is ubuntu 14.04 I think....
comment:127 Changed 11 months ago by
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 --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.
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 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:
#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