Implement period_lattice for elliptic curves over RealField, ComplexField, etc.

[user202729]

Implement E.period_lattice() method for elliptic curves over other fields.

(The code is mostly already there, just need minor adaptation.)

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion. (not aware of one)
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

[github-actions]

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

[user202729]

The failing test looks unrelated to the changes, I think.

[tscrim]

@JohnCremona Perhaps you might be interested in this.

[JohnCremona]

I'll look at this next week

[user202729]

Caveat:

• A EllipticCurve_number_field inherits from EllipticCurve_field.
• Given an instance of EllipticCurve_field, you can call .period_lattice() on it with no argument and it will return something.
• This contract is not satisfied with an EllipticCurve_number_field object, violate Liskov substitution principle.

What do you think?

One way is to default embedding=None for EllipticCurve_number_field.period_lattice(), but then we need to think what to do in that case.

• use E.base_field().coerce_embedding() (suggested in EllipticCurve.period_lattice() should automatically use the canonical embedding for embedded number fields? #38363 ). This breaks backwards compatibility, but I think this is the best option.

• give an error: this is not the current behavior with embedding=None (thus also break backwards compatibility), but would be the safest option.

• follow the current behavior of PeriodLattice_ell constructor when embedding=None:

  • embedding -- (default: None) an embedding of the base
    field K of E into a real or complex field. If None:
    • use the built-in coercion to \RR for K=\QQ;
    • use the first embedding into \RR given by
      K.embeddings(RealField()), if there are any;
    • use the first embedding into \CC given by
      K.embeddings(ComplexField()), if K is totally complex.

[user202729]

@JohnCremona Can I get a review? Thanks.

[JohnCremona]

I am happy with this, assuming that old tests still pass. It is a good idea to change the behavious over number fields with no specified embedding: the choice to use the "first" one was quite random, probably introduced by me when I first did this. Certainly, when there is a default embedding for the number field, that is the one to use.

[JohnCremona]

I think it is up to the original author to fix the failing tests, which do not look serious. My positive review was only in principle, once these things have been sorted out.

[user202729]

Indeed the failing tests are not serious (it's just one error message over another). I didn't notice the test failure after merging the branch, sorry.

Changing the behavior to only use coerce_embedding instead of arbitrarily pick the first embedding sounds like a good idea, I will implement it later.

[user202729]

I decide to just fix the failing tests by relax the thing being tested for.

The part of using canonical embedding feels like a behavioral change ⟹ deprecation period ⟹ etc. and it's probably easiest to handle it in a separate pull request.

[JohnCremona]

There are still some failing tests being flagged. Do they fail when run locally?

[user202729]

I… use GitHub Actions to run the tests, sorry. Should really get around to figure out how to run it locally…