Update the rank function of elliptic curves to use ellrank in pari #35626

chriswuthrich · 2023-05-08T02:17:57Z

📚 Description

Pari has now a very good in-built algorithm to determine the rank and a set of generators of the Mordell-Weil group of elliptic curves over the rational. It is, as far as I understand, a port of Denis Simon's script ellQ.gp. It performs a 2-descent and uses the Cassels Tate pairing to conclude the rank in many cases - when Sha has no 4-torsion elements.
The functionality to search rational points is also very quick.

📝 Checklist

[x] The title is concise, informative, and self-explanatory.
[x] The description explains in detail what this PR is about.
[x] I have linked a relevant issue or discussion.
[x] I have created tests covering the changes.
[x] I have updated the documentation accordingly.

⌛ Dependencies

allow non-minimal equation for padic heights #35614 a pull request that fixes, among other things, the documentation in the files changed here.
To avoid problems, I based it on that PR.

chriswuthrich · 2023-05-09T07:46:22Z

This is ready for review.

I implemented that .rank() and .prove_BSD() still take mwrank by default, but they have the option to take algorithm=pari now. Instead .selmer_bound() and rank_bound() now take pari by default, but allow mwrank as an option. For rank_bound this is justified as the implementation in pari is better in the sense that it also uses the Cassels-Tate pairing and hence determines a smaller upper bound in some cases.
I made simon_two_descent deprecated. Only for elliptic curves over Q, it is still available over number fields.

Maybe it is better to take pari as the default in all these, but I do not have enough experience in comparing the two. Obvioulsy @JohnCremona is asked for comment on this here :)

However, this question may be decided later with a small change. My choice here already improves sage and it could be the optimal choice...

JohnCremona · 2023-05-09T09:47:46Z

I will review this. When Bill Allombert was implementing all this in pari he corresponded frequently with me, and I know that he did a really excellent job. As a result of that, mwrank was improved a bit too (which Sage has alreay benefited from), for example in saturation of points.

JohnCremona

I have made a few suggestions. By the way, the code changes here are on top of those in your other PR, so I have not re-reviewed those parts -- but it would have been a bit better to have made a fresh branch for this.

Anyway -- thanks a million for doing this! I have been hoping that someone would ever since pari's ellrank has been available. Also, I would be quite happy for 'pari' to replace 'mwrank' as default.

JohnCremona · 2023-05-09T10:03:41Z

src/sage/schemes/elliptic_curves/BSD.py

+
+    - ``E`` -- an elliptic curve
+
+    OUTPUT:


Say that the output is a tuple of 5 elements, ...

JohnCremona · 2023-05-09T10:08:29Z

src/sage/schemes/elliptic_curves/BSD.py

+    # dim Sha[2] = dim Sel2 - rank E(Q) - dim tors
+    # rank_upper_bd = dim Sel_2 - dim tors - s
+    sha_upper_bd = rank_upper_bd - len(gens) + s
+    return len(gens), rank_upper_bd, s, sha_upper_bd, gens


Why return len(gens) instead of pari's 'lower', which you don't use? Can the lower rank bound be greater than the number of gens? But if pari increases the lower rank bound by 1 conditional on parity then I approve of using len(gens) instead.

That is my understanding of pari, although their documentation does not mention that the lower bound is conditional on the finiteness of the 2-primary part of the Tate-Shafarevich group.

In line 1953 of file ellrank.c the function ell2selmer, we have if (odd(dim - nbpoints)) mwrank++;

I was planning to write to the pari-gp list asking about this (and whether they have plans to port the code for the number fields, too.

OK -- let's just put a comment in that we don't use pari's lower bound as that is sometimes incremented by 1 based on a standard conjecture.

JohnCremona · 2023-05-09T10:11:09Z

src/sage/schemes/elliptic_curves/ell_rational_field.py

+            This function is deprecated as the functionality of
+            Simon's script for elliptic curves over the rationals
+            has been ported over to pari.
+            Use :meth:`.rank` with the keyword ``algorithm='pari;`` instead.


sytax of this line looks dodgy, and I think the semicolon should be another single quote.

JohnCremona · 2023-05-09T10:12:05Z

src/sage/schemes/elliptic_curves/ell_rational_field.py

-             proof=None):
-        """
+             proof=None,
+             pari_effort=0):


Is 0 really the best default?

Good question. It is the default in pari. I am amazed how many points are already found with effort 0. If I understand correctly, the algorithm calculates the 2-Selmer group first, then searches until either the effort bound limits it or enough points are found. In this way having a larger default value wouldn't harm the speed of the easy examples. However, maybe it is best to leave it to the user to do it with minimum effort first and then to increase. The way I implemented it the points found already in the first run are passed over to pari's second run.

Good answer! Leave it as 0.

JohnCremona · 2023-05-09T10:12:49Z

src/sage/schemes/elliptic_curves/ell_rational_field.py

@@ -2038,14 +2061,16 @@ def rank(self, use_database=True, verbose=False,
            sage: E.minimal_model().rank()
            1

-        A large example where mwrank doesn't determine the result with certainty::
+        A large example where mwrank doesn't determine the result with certainty, but pari does::


JohnCremona · 2023-05-09T10:16:38Z

src/sage/schemes/elliptic_curves/ell_rational_field.py

@@ -2218,13 +2283,18 @@ def gens(self, proof=None, **kwds):
            :meth:`~gens_certain` method to find out afterwards
            whether the generators were proved.

-        IMPLEMENTATION: Uses Cremona's mwrank C library.


It's actually C++, but this has been here for years...

JohnCremona · 2023-05-09T10:17:56Z

src/sage/schemes/elliptic_curves/ell_rational_field.py

+            RuntimeError: generators could not be determined. So far we found []. Hint: increase pari_effort.
+            sage: E.gens(use_database=False, algorithm="pari",pari_effort=10)
+            [(-166136231668185267540804/2825630694251145858025 : 167661624456834335404812111469782006/150201095200135518108761470235125 : 1)]
+


I like the last two examples -- are they fast enough or should they be tagged 'long time'?

The example -127^2 and effort 4 is done in 100ms on my old laptop. The last -157^2 with effort 10 takes seconds, so I put long time to that.

JohnCremona · 2023-05-09T10:20:26Z

src/sage/schemes/elliptic_curves/ell_rational_field.py

@@ -2333,7 +2426,26 @@ def _compute_gens(self, proof,
            except RuntimeError:
                pass
        # end if (not_use_mwrank)
-        if algorithm == "mwrank_lib":
+        if algorithm == "pari":


This block repeats lines 2210-2227 rather closely. Would it be possible to have a helper function used by both? The helper would not itself raise a runtime error, but that condition would be caught by the calling function.

Do you insist? I am too lazy to put it into one for just two cases. I agree it might be better.

JohnCremona · 2023-05-09T10:21:45Z

src/sage/schemes/elliptic_curves/ell_rational_field.py

+                ep = self.pari_curve()
+                lower, upper, s, pts = ep.ellrank()
+                T = self.torsion_subgroup().invariants()
+                tor = sum(x%2==0 for x in T)


You could use self.two_torsion_rank(), provided just for this!

JohnCremona · 2023-05-09T10:24:07Z

src/sage/schemes/elliptic_curves/hom_velusqrt.py

@@ -1,7 +1,7 @@
 r"""
-√élu algorithm for elliptic-curve isogenies


I think that the funny square-root character in place of V is intentional, as that is what @yyyyx4 and friends call the improved Velu algorithm 'velusqrt'. So perhaps revert all these?

Oh, I didn't know. I saw all of these (find them ugly and possibly insulting to Monsieur Vélu qui n'est pas un élu). I am personally against it in headers, as it just looks like a bug. If they want to use it as a curiosity in some of the documentation I am not against, it, but I would still want most of them to be reverted. Sorry to be spoil fun.

@yyyyx4 How strongly do you feel about this? Every time I see it I think there is something wrong with my computer screen!

Well, √élu is the name given to the algorithm by its authors (not me). Other names that have appeared in the literature are all variants of "square‑root Vélu", with varying choices of punctuation, capitalization, accentuation, abbreviation, and word order. I obviously prefer √élu, but most of the other variants are acceptable to me. Identifying this algorithm with Vélu's seems wrong, though.

Thanks for the comments. (Due to my bad choice of basing this issue on top of the other, the change of Vélu actually belongs to the dependency and not to this pull request. I will correct it there.)

I will of course correct this and I am sorry that I did this change without looking closer.
I will use "square-root Vélu". If there are other who prefer keeping it in the documentation and in the _repr_ then they should comment here (or better on the other pull request). I don't have a strong feeling and I am ok to revert to √élu.

…nto trac34790_2

chriswuthrich · 2023-05-09T23:04:34Z

I still have not decided which algorithm to take as default. I think they are similar in speed but pari determines the rank more often.

Here some very naive timing on my old laptop:

curves	#curves	mwrank_lib	pari
1<N<=1000	5113	2.9 s	2.94 s
8888<=N<=10000	7444	4.46 s	4.49 s
11111<=N<=12345	8201	5.4 s	4.85 s
111111<=N<=112345	7609	5.02 s	4.51 s
[1,B] with 1<=B<=1000	1000	40.2 s but failed in 31 cases	2min 17s
[3,B] with 1<=B<=1000	1000	40.7 s but failed in 35 cases	2min 21s
[A,1] with 1<=A<=1000	1000	59.7 s but failed in 67 cases	3min 23s but failed in 4 cases

Based on this I would suggest to change for pari as default. But I welcome any other opinion on this.

JohnCremona · 2023-05-10T07:46:56Z

I think that I have re-reviewed and checked that the opints I raised have all been resolved, but I am not certain. GitHub's reviewing system is very sophisticated but also quite confusing. I did not see how it was possible for me as a reviewer to mark each point I had raised as "resolved".

About the default, let's leave it as it is for now. Changing it later would be easy.

chriswuthrich · 2023-05-10T09:23:54Z

I agree with leaving the default as it is. I am not confident at all about my rough timings above. I will check with pari if they have experience in that respect.

Many thanks, John, for the helpful comments and the detailed review of the two tickets. Now that I am getting more used to this interface here, I am contactable for any reviews myself.

vbraun · 2023-05-24T22:28:37Z

Merge conflict

JohnCremona · 2023-06-01T11:10:21Z

Same as #35614 -- I'll give it +ve review when tests have passed

chriswuthrich · 2023-06-01T21:08:22Z

Probably not the canonical solution, but I updated the dependant branch by resolving the merge conflicts with develop there, then I rebased this branch on the new version of the dependent branch. (Sorry if that is not good, but I think the history is cleaner this way.) [[ In the future I will not pile pull requests on top of each other, promised...]]

github-actions · 2023-06-04T10:50:15Z

Documentation preview for this PR (built with commit 0f8b28a) is ready! 🎉

#35626 introduced a test failure on some machines as some tests return random choices of generators of Mordell-Weil groups for elliptic curves. This adds "random" to these tests and add some tests that do test if the returned answer is correct. This is only possible when the rank is 1 (and the returned point must be in a finite set). For larger ranks at best we can test if the number of generators is ok and if the points are saturated. URL: #35787 Reported by: Chris Wuthrich Reviewer(s):

tornaria · 2023-08-28T01:02:27Z

src/sage/schemes/elliptic_curves/ell_rational_field.py

+            sage: E = EllipticCurve([0,2429469980725060,0,275130703388172136833647756388,0])
+            sage: len(E.gens(algorithm="pari"))
+            14


This test takes 50s, and brings testing the whole file (around 900 tests) from 22s in sagemath-10.0 to 73s in sagemath-10.1.

This doesn't seem ok even for --long.

This looks like an example where the old default (mwrank) is better than the newly implemented pari code.

Indeed, mwrank takes 13s while pari takes 51s. The default seems to be still mwrank.

In any case, I would suggest that this test be marked "# not tested, too long" or something (or be replaced by a smaller rank example which takes <1s).

Fully agree. It should stay in the documentation as a complicated example, but not be part of the tests. (Just back from holidays and fairly busy for a while) Please feel free to change this, I will do it but it will take a while.

@chriswuthrich

…too long A doctest introduced in sagemath#35626 takes ~50s on current hardware (for comparision, doctesting the whole of `src/sage/schemes/elliptic_curves/ell_rational_field.py` takes ~22s for 889 tests). This is too long even for `--long`, but the example is still interesting, so we just mark it `# not tested`. ### 📝 Checklist - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. CC: @chriswuthrich (should be a trivial review 🙏) URL: sagemath#36195 Reported by: Gonzalo Tornaría Reviewer(s):

chriswuthrich and others added 7 commits May 5, 2023 12:21

trac 34790: added function for finding correct multiple

2868775

trac 34790: integrate it

1ff4524

trac 34790: docstrings

3612d72

trac 34790: docstring fixing

caa0c9d

trac 34790: Q to QQ in dostrings

de07565

trac 34790: raw strings

6aabc74

Merge branch 'develop' into trac34790_2

66b235a

fchapoton added the c: elliptic curves label May 9, 2023

chriswuthrich marked this pull request as ready for review May 9, 2023 07:36

JohnCremona self-requested a review May 9, 2023 09:45

JohnCremona self-assigned this May 9, 2023

JohnCremona suggested changes May 9, 2023

View reviewed changes

chriswuthrich added 3 commits May 9, 2023 20:18

add missing doctests in padics.py

253491e

Merge branch 'trac34790_2' of https://github.com/chriswuthrich/sage i…

34f8dd4

…nto trac34790_2

revert velu to square-root velu

bb53db8

chriswuthrich mentioned this pull request May 9, 2023

allow non-minimal equation for padic heights #35614

Merged

JohnCremona approved these changes May 10, 2023

View reviewed changes

JohnCremona added the s: positive review label May 10, 2023

vbraun added the s: needs work label May 24, 2023

kwankyu removed the s: positive review label May 28, 2023

tiny merge conflict

c9acc17

delete one extra empty line

2d9b3a2

chriswuthrich added 6 commits June 1, 2023 20:27

integrate ellrank for elliptic curves over QQ

138b9cf

issue 35621: deprecate simon_two_descent over QQ

c75835b

issue 35621: change prove_bsd

2ff83fb

issue 35621: saturation for points from pari

b5d59cd

adjust rank_bound and selmer_rank

b4f0de6

revievers suggested changes

3ff268a

chriswuthrich force-pushed the pariell branch from 3c904ff to 3ff268a Compare June 1, 2023 21:04

Merge branch 'develop' into pariell

0f8b28a

JohnCremona added s: positive review and removed s: needs work labels Jun 6, 2023

vbraun merged commit 86d2be0 into sagemath:develop Jun 11, 2023

mkoeppe added this to the sage-10.1 milestone Jun 11, 2023

mkoeppe removed the s: positive review label Jun 11, 2023

chriswuthrich mentioned this pull request Jun 17, 2023

adjust random tests for generators of elliptic curves #35787

Merged

chriswuthrich deleted the pariell branch July 4, 2023 12:05

tornaria reviewed Aug 28, 2023

View reviewed changes

tornaria mentioned this pull request Sep 5, 2023

schemes/elliptic_curves: disable a test that takes too long #36195

Merged

3 tasks

		@@ -1,7 +1,7 @@
		r"""
		√élu algorithm for elliptic-curve isogenies

Update the rank function of elliptic curves to use ellrank in pari #35626

Update the rank function of elliptic curves to use ellrank in pari #35626

Conversation

chriswuthrich commented May 8, 2023 • edited Loading

📚 Description

📝 Checklist

⌛ Dependencies

chriswuthrich commented May 9, 2023 • edited Loading

JohnCremona commented May 9, 2023

JohnCremona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriswuthrich commented May 9, 2023

JohnCremona commented May 10, 2023

chriswuthrich commented May 10, 2023

vbraun commented May 24, 2023

JohnCremona commented Jun 1, 2023

chriswuthrich commented Jun 1, 2023

github-actions bot commented Jun 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriswuthrich commented May 8, 2023 •

edited

Loading

chriswuthrich commented May 9, 2023 •

edited

Loading