add .twists() to EllipticCurve_finite_field

[yyyyx4]

Fixes #34782.

[codecov-commenter]

Codecov Report

Base: 88.60% // Head: 88.58% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (f99dc67) compared to base (c000c95).
Patch coverage: 92.78% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #34981      +/-   ##
===========================================
- Coverage    88.60%   88.58%   -0.02%     
===========================================
  Files         2136     2140       +4     
  Lines       396142   397058     +916     
===========================================
+ Hits        350990   351744     +754     
- Misses       45152    45314     +162     

Impacted Files	Coverage Δ
src/sage/schemes/elliptic_curves/ell_field.py	80.92% <ø> (ø)
...c/sage/schemes/elliptic_curves/ell_finite_field.py	88.71% <92.78%> (+1.14%)	⬆️
src/sage/groups/affine_gps/euclidean_group.py	88.88% <0.00%> (-11.12%)	⬇️
src/sage/quadratic_forms/random_quadraticform.py	90.00% <0.00%> (-6.00%)	⬇️
src/sage/interfaces/qsieve.py	71.30% <0.00%> (-2.61%)	⬇️
src/sage/groups/affine_gps/affine_group.py	96.62% <0.00%> (-2.25%)	⬇️
src/sage/modular/local_comp/liftings.py	98.33% <0.00%> (-1.67%)	⬇️
src/sage/cpython/_py2_random.py	74.79% <0.00%> (-1.66%)	⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py	94.09% <0.00%> (-1.58%)	⬇️
src/sage/modular/hecke/algebra.py	94.65% <0.00%> (-1.07%)	⬇️
... and 70 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[yyyyx4]

The test failure is yet another instance of #35017.

[JohnCremona]

Thanks, I wonder why I did not implement this years ago. Review comments:

1. Do we have to use random elements? You could set D = K.multiplicative_generator()**((q-1)/deg). I guess that the method as written will be faster for large fields.
2. I would love to include characteristics 2 and 3. These are not hard for j!=0=1728, and even for those cases you can find in the code (+ examples and comments) complete lists of the twists.

[yyyyx4]

1. Indeed, calling .multiplicative_generator() ends up factoring $q-1$. In general I care more about efficient algorithms than about deterministic algorithms; determinism can (in practice) always be achieved using set_random_seed(). How about adding an optional deterministic= flag that will use the root of unity?
2. I won't have time to work on this for another few weeks, but I can add it to the ever-growing TODO list...

[JohnCremona]

For my 2nd comment, if you look in ell_field.py you will see that E.quadratic_twist() gives the unique twist in char. 2 provided j is nonzero:

sage: E = EllipticCurve(GF(64), j=1); E
Elliptic Curve defined by y^2 + x*y = x^3 + 1 over Finite Field in z6 of size 2^6
sage: E.quadratic_twist()
Elliptic Curve defined by y^2 + x*y = x^3 + z6^3*x^2 + 1 over Finite Field in z6 of size 2^6
sage: E = EllipticCurve(GF(81), j=1); E
Elliptic Curve defined by y^2 = x^3 + x^2 + 2 over Finite Field in z4 of size 3^4
sage: E.quadratic_twist()
Elliptic Curve defined by y^2 = x^3 + (z4^3+z4^2+2*z4)*x^2 + (2*z4^3+z4^2+2) over Finite Field in z4 of size 3^4

For the case char=2 or 3 and j=0, my old code (which was moved to a separate file cardinality.py when it was no longer used by default) has useful examples in lines 54-96: there are 4 cases (despite 0=1728!) since the number of twists depends on the parity of the field degree.

Here's an idea @yyyyx4: I can add these extra cases and make a PR to your branch; if you like my changes you can merge that and this PR will then automatically update.

[yyyyx4]

@JohnCremona Sounds great!

[JohnCremona]

@JohnCremona Sounds great!

OK, so I did

git fetch upstream pull/34981/head:ecff-twists
git co ecff-twists

and am working on it. When I am done I will push to my github fork and make a PR from that to your @yyyyx4 .

(Adding detail here is this sort of workflow is not familiar to all.)

[JohnCremona]

@yyyyx4 I have been working on this, not quite finished. The only source I know for the hard cases (char. p=2,3 and j=0) is unpublished online notes of Ian Connell (with only a sketch of the details), I did once work through it myself but have just redone that. I'll add a citation to Connell.

Also, the PR I will make to your branch will change a few things in the cases you already implemented and I want to run that past you. I want to have separate functions listing the isomorphism classes of curves with j=0 and j=1728 (these are quite long in the hard cases just mentioned) which I think will be cleaner; then the twists method can just call that but replace one of the curves in the standard list with self and move it to the front of the list.

I am still a bit bothered about the non-determinacy: in the cases where there are 4 quartic or 6 sextic twists, since the group K*/K*^4 or K*/K*^6 has two generators the output curves can some in one of two different orders, even up to isomorphism. As a partial solution which is cost-free, and actually saves work in many cases, I will do the following:

• when j=0 but q=2 mod 3 so there is only a quadratic twist (not 6 sextic twists) we can take D=-3;
• when j=1728 but q=3 mod 4 so there is only a quadratic twist we can take D=-1;
• when we use random elements we can start with D=K.gen() since for many fields by default that will be a fixed primitive generator and will pass the test.

I hope to finish this today.

[JohnCremona]

@yyyyx4 My update of your branch is at https://github.com/JohnCremona/sage/tree/ecff-twists. At the end I did a rebase on 9.8, with the unfortunate side-effect that making a PR to your branch is harder, but I could make a new PR to the main develop branch.

The new twists method plus helper function all with doctests are now nearly 500 lines, sorry about that, but it does handle all cases cleanly (and a lot of the lines are docstrings).

[yyyyx4]

Thanks, it looks very nice! I've merged your branch into this one. The doctests were failing sometimes due to the remaining non-determinism. They are fixed for now, but perhaps we want to simply implement both the deterministic and potentially non-deterministic algorithms and allow users to choose between them with an optional deterministic= flag?

[JohnCremona]

Thanks. I thought I had added the #random tag on the tests where the results were not deterministic, but I must have missed some. Note that sorting the curves will not always be sufficient.

I'm not sure that users will care enough about the results being deterministic to make it worth implementing that.

Could you add my name to the authors after yours ;) ?

[yyyyx4]

Could you add my name to the authors after yours ;) ?

Of course, already done, in the associated issue #34782. The git history also remembers your original commit including metadata. But I'm not sure if this is how shared authorship is supposed to be handled now...

[JohnCremona]

Could you add my name to the authors after yours ;) ?

Of course, already done, in the associated issue #34782. The git history also remembers your original commit including metadata. But I'm not sure if this is how shared authorship is supposed to be handled now...

I meant, at the top of the ell_finite_field.py file.

Nice to get a notification that the documentation is now built and viewable! The relevant page is https://deploy-preview-34981--sagemath-tobias.netlify.app/reference/arithmetic_curves/sage/schemes/elliptic_curves/ell_finite_field.html. This reveals some bad docstrings (all my fault), one being \F (should it be \FF?), also two curve equations in sage.schemes.elliptic_curves.ell_finite_field.curves_with_j_0

Would you fix those or shall I?

[yyyyx4]

Oh, now I see, sorry. Done, and also (hopefully) fixed the docstrings.

[JohnCremona]

Oh, now I see, sorry. Done, and also (hopefully) fixed the docstrings.

Thanks. I'll wait to see that the docstrings look good now, then this is ready to go -- but we probably need a 3rd party to review it, or does the old convention that it is OK for both of us to review joint work still apply?

[github-actions]

Documentation preview for this PR is ready! 🎉
Built with commit: f99dc67

[yyyyx4]

If jointly reviewing joint work was okay on Trac, it should probably also be okay here? Either way, I'm happy with the changes.

[JohnCremona]

If jointly reviewing joint work was okay on Trac, it should probably also be okay here? Either way, I'm happy with the changes.

OK then...