work on Simon two-descent

[fchapoton]

using pari library rather than gp interface ; also avoiding direct use of sage_eval

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.

[github-actions]

Documentation preview for this PR (built with commit 52b0994; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[tscrim]

Just one question. Either way you decide, you can set a positive review.

[fchapoton]

thanks for the review !

[vbraun]

I'm getting exactly the same fail as the CI, could you please run the tests before setting it to positive review instead of wasting my time?

[tscrim]

@vbraun Sorry about that; the main tester had a bad run and I ignore the Conda testing because that is almost always broken. When I ran the tests locally, they passed; perhaps I used the wrong copy of Sage when running the tests.

@fchapoton I get a failure in src/sage/schemes/elliptic_curves/ell_number_field.py # 1 doctest failed from a change in the verbose output.

[fchapoton]

there remains an issue in ell_rational_field too

[tscrim]

I cannot reproduce those failures locally.

[fchapoton]

ok, this seems to work well right now, both here and on my machine. I will set back to "needs review".

[fchapoton]

I still have a failing doctest in src/sage/schemes/elliptic_curves/ell_rational_field.py about insufficient Pari effort..

[fchapoton]

This is random, triggered by the exact line
sage -t --long --random-seed=11793909189628564618859204544679139487 src/sage/schemes/elliptic_curves/ell_rational_field.py

(this exact command works in vanilla sage 10.5.beta3)

[tscrim]

That seems strange. Do you know if the PARI portions are nondeterministic?

[fchapoton]

no idea. We would need advice from some pari developers.

Also, maybe I should keep the init() method to load the scripts just once ?

[fchapoton]

@AurelPage : would you please have any suggestion to make about the failure with "insufficient pari effort" here ?

The elliptic curve is deterministic, but the algo in pari to find the generators is certainly random as we tag the result on the next line as #random

@JohnCremona , is there is a reasonable way to make this test more robust ?

[JohnCremona]

@fchapoton I don't know what "this test" is.

This may not be what you wanted to hear, but here goes. Many years ago I suggested to Denis Simon that he could implement 2-descent in GP, and he did, with different code for Q and other number fields and for the different cases (depending on how much 2-torsion is rational). That code was incrementally improved over several years, though not at all recently. Much more recently, Bill Allombert wrote a completely new 2-descent implementation in pari itlef (i.e. in libpari) which Sage now uses by default. This makes Simon's gp scripts completely redundant for elliptic curves over Q. We should neither use nor support it, and we should make obsolete the simon_two_descent() method as soon as possible. There is a comment in the docstring for simon_two_descent() over Q which says that it is deprecated, so I hope it will be scrapped soon.

For elliptic curves over other number fields, Simon's script still has some use, but I very much hope that libpari will take that over soon to make it equally redundant.

Now I can answer a more specific question -- bear in mind that the main point of 2-descent is to give generators for E(K)/2E(K) and (and always for generators of a finite abelian group) they are not unique.

[AurelPage]

@fchapoton I could not reproduce the test failure locally (something about the sphinx distribution, which did not occur on develop).
The pari function ellrank definitely uses randomness (src/basemath/ellrank.c:1993 on master). It searches for points of small height on 2-covers of the curve, where the 2-cover is chosen randomly from the 2-Selmer group. The effort parameter controls how many 2-covers it tries, and the height bound. It can definitely happen that for some bad seeds, the function will only try 2-covers where the height bound is insufficient. I think that there should be an effort parameter that will ensure success independently of the seed, because all 2-covers that could be tried will have a small enough point. If you give me the input that is sent to pari (or the line number in ell_rational_field.py), I can investigate the particular example.
In addition, I second @JohnCremona about the fact that Simon's script for elliptic curves over Q is superseded by ellrank. It would be nice to extend the pari functionality to number fields, but we are not working on it at the moment.

[fchapoton]

Thanks a lot for the feedback. I understand your points.
I have chosen to remove the problematic doctest around line 2400 in ell_rational.py, as it was just showing muscles, duplicating the previous line but with a more difficult curve,

[JohnCremona]

Thanks a lot for the feedback. I understand your points. I have chosen to remove the problematic doctest around line 2400 in ell_rational.py, as it was just showing muscles, duplicating the previous line but with a more difficult curve,

Fair enough. I may have been the one who added that test: it soves the congruent number problem for n=157, which used to be hard :). I think the point was to have an example where the default effort did not succeed, but increasing it does.

[fchapoton]

could someone please give a positive review, if you agree ?

[JohnCremona]

I just gave this a positive review. I did not pull the branch, rebuild, test and the rebuild again after switching back. That used to be reasonable but now takes too long.

[fchapoton]

thanks a lot. I think looking at the CI is now reasonable for that.