insufficient precision in scaling elliptic curves over number fields by units

[8f946f1e-365d-41e6-b74d-abfb047b3598]

<this was first reported on the sage-devel google group: https://groups.google.com/g/sage-devel/c/s0B7OqpB0KU/m/18eHSiRWAAAJ ; as requested I'm opening this ticket>

I am running into an issue with computing isogeny classes of elliptic curves over number fields.

Here is what I entered:

K.<a> = QuadraticField(4569)
myJ = 46969655/32768
E = EllipticCurve(j=K(myJ))
C = E.isogeny_class()

This raises a KeyError in the first instance, which seems to be handled via a direct call to IsogenyClass_EC_NumberField, but this then raises a ValueError: Cannot convert infinity or NaN to Sage Integer. See the above linked discussion for the full stacktrace.

This error has occurred on sage-9.4 on a system whose uname -a is Linux LEGENDRE 5.4.0-91-generic #102-Ubuntu SMP Fri Nov 5 16:31:28 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux, as well as on sage-9.7-beta5 on Linux Barinder 5.4.0-121-generic #137-Ubuntu SMP Wed Jun 15 13:33:07 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux.

Changing the field K to many other quadratic fields does not yield any error. However I did also notice this error with many j-invariants in QuadraticField(6537), so it somehow seems to be base-field dependent.

Two issues are fixed in this ticket: (1) computing the isogeny class could fail for a curve defined by a non-integral model; (2) precision was not handled well in scaling equations by units.

CC: @JohnCremona

Component: elliptic curves

Keywords: isogeny-classes

Author: John Cremona

Branch/Commit: 783dbc3

Reviewer: David Lowry-Duda

Issue created by migration from https://trac.sagemath.org/ticket/34174

[JohnCremona]

comment:1

I'm sure it's because the fundamental unit is very skew:

sage: u, = K.units()
sage: embs = K.embeddings(RealField(100))
sage: [e(u) for e in embs]
[-3.9113242967798985503709688068e34, -16384.000000000000000000000000]

and the fix will likely involve increasing precision at some point.

As a work around, which I would recommend anyway for curves with rational j-invariant:

sage: K.<a> = QuadraticField(4569)
sage: myJ = 46969655/32768
sage: E = EllipticCurve(j=myJ)
sage: EK = E.change_ring(K)
sage: C = EK.isogeny_class(minimal_models=False)

It should be possible to do E.isogeny_class(minimal_models=False) with E as you defined it, but this raises a different error. I'll fix that one too.

[JohnCremona]

comment:2

The KeyError observed is harmless, when you ask for the isogeny class it first checks to see if it already has it, and only computes it if not.

For the second issue, line 1523-6 of gal_reps_number_field in the function reducible_primes_Billerey are

    K = E.base_field()
    DK = K.discriminant()
    ED = E.discriminant().norm()
    B0 = ZZ(6*DK*ED).prime_divisors()  # TODO: only works if discriminant is integral

and the last line can raise an error when E is not an integral model. I intend to avoid this by making sure that the various Billerey bound functions (which I wrote) are only called on integral models. [done, testing.]

The first (main) issue is in ell_number_field in the method _scale_by_units(). There's a line in there

        prec = 1000  # lower precision works badly!

which is revealing. The example reported here is clearly one where 1000 is not enough. The lazy way out is to double that, but clearly it is better either to try to work out a suitable precision, or to wrap this in a loop which steadily increases the precision. Or something. I'll see what I can do -- there have been many other places where similar issues have arisen when working with elliptic curves over number fields.

[JohnCremona]

Author: John Cremona

[JohnCremona]

Commit: 783dbc3

[JohnCremona]

Description changed:

--- 
+++ 
@@ -18,3 +18,5 @@
 This error has occurred on sage-9.4 on a system whose `uname -a` is `Linux LEGENDRE 5.4.0-91-generic #102-Ubuntu SMP Fri Nov 5 16:31:28 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux`, as well as on sage-9.7-beta5 on `Linux Barinder 5.4.0-121-generic #137-Ubuntu SMP Wed Jun 15 13:33:07 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux`.

 Changing the field K to many other quadratic fields does not yield any error. However I did also notice this error with many j-invariants in `QuadraticField(6537)`, so it somehow seems to be base-field dependent.
+
+Two issues are fixed in this ticket: (1) computing the isogeny class could fail for a curve defined by a non-integral model; (2) precision was not handled well in scaling equations by units.

[JohnCremona]

Branch: u/cremona/34174

[JohnCremona]

comment:3

Both issues are fixed in branch u/cremona/34174, with two doctests added.

---

New commits:

783dbc3

#34174: fix precision issue in scaling elliptic curves by units

[8f946f1e-365d-41e6-b74d-abfb047b3598]

Description changed:

--- 
+++ 
@@ -18,5 +18,3 @@
 This error has occurred on sage-9.4 on a system whose `uname -a` is `Linux LEGENDRE 5.4.0-91-generic #102-Ubuntu SMP Fri Nov 5 16:31:28 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux`, as well as on sage-9.7-beta5 on `Linux Barinder 5.4.0-121-generic #137-Ubuntu SMP Wed Jun 15 13:33:07 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux`.

 Changing the field K to many other quadratic fields does not yield any error. However I did also notice this error with many j-invariants in `QuadraticField(6537)`, so it somehow seems to be base-field dependent.
-
-Two issues are fixed in this ticket: (1) computing the isogeny class could fail for a curve defined by a non-integral model; (2) precision was not handled well in scaling equations by units.

[8f946f1e-365d-41e6-b74d-abfb047b3598]

comment:4

Thanks for looking into this John -- I'll try the workaround you suggested above to get my code working entirely in Sage. (My previous workaround was to use ellisomat in PARI/GP; this is related to this paper: https://arxiv.org/abs/2206.08891, whose results we're currently expanding to many other quadratic fields, including QuadraticField(4569).)

[8f946f1e-365d-41e6-b74d-abfb047b3598]

Changed author from John Cremona to none

[8f946f1e-365d-41e6-b74d-abfb047b3598]

Changed branch from u/cremona/34174 to none

[8f946f1e-365d-41e6-b74d-abfb047b3598]

Changed commit from 783dbc3 to none

[8f946f1e-365d-41e6-b74d-abfb047b3598]

comment:5

oops, it looks like we submitted those comments at the same time, apologies for overwriting the changes you put in on the ticket! i'll revert those

[8f946f1e-365d-41e6-b74d-abfb047b3598]

Commit: 783dbc3

[8f946f1e-365d-41e6-b74d-abfb047b3598]

Branch: u/cremona/34174

[8f946f1e-365d-41e6-b74d-abfb047b3598]

Author: John Cremona

[8f946f1e-365d-41e6-b74d-abfb047b3598]

Description changed:

--- 
+++ 
@@ -18,3 +18,5 @@
 This error has occurred on sage-9.4 on a system whose `uname -a` is `Linux LEGENDRE 5.4.0-91-generic #102-Ubuntu SMP Fri Nov 5 16:31:28 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux`, as well as on sage-9.7-beta5 on `Linux Barinder 5.4.0-121-generic #137-Ubuntu SMP Wed Jun 15 13:33:07 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux`.

 Changing the field K to many other quadratic fields does not yield any error. However I did also notice this error with many j-invariants in `QuadraticField(6537)`, so it somehow seems to be base-field dependent.
+
+Two issues are fixed in this ticket: (1) computing the isogeny class could fail for a curve defined by a non-integral model; (2) precision was not handled well in scaling equations by units.

[JohnCremona]

comment:8

Replying to @BarinderBanwait:

oops, it looks like we submitted those comments at the same time, apologies for overwriting the changes you put in on the ticket! i'll revert those

It's OK, you don't need to revert anything.

[JohnCremona]

comment:9

Replying to @JohnCremona:

Replying to @BarinderBanwait:

oops, it looks like we submitted those comments at the same time, apologies for overwriting the changes you put in on the ticket! i'll revert those

It's OK, you don't need to revert anything.

--but it would be good if you were able to review this to say that the problem is fixed!

[8f946f1e-365d-41e6-b74d-abfb047b3598]

comment:10

Replying to @JohnCremona:

Replying to @JohnCremona:

Replying to @BarinderBanwait:

oops, it looks like we submitted those comments at the same time, apologies for overwriting the changes you put in on the ticket! i'll revert those

It's OK, you don't need to revert anything.

--but it would be good if you were able to review this to say that the problem is fixed!

Sure! I'll checkout your branch, rebuild sage, and test that it now works

[JohnCremona]

comment:11

Replying to @BarinderBanwait:

Sure! I'll checkout your branch, rebuild sage, and test that it now works

At some point the build/testbots will report that tests pass, so you don't have to rebuild sage with the patch. What you (or someone) can do is to look at the code changes and say that they are mathematically sensible, and also to note that the added tests do mean that when they pass it is a sign that the original reported problem is fixed. You can see the code changes by clicking on the branch name at the top of the ticket. This is a lot quicker than rebuilding Sage yourself (though you might want to anyway in order to be able to use the fixed version sooner).

[8f946f1e-365d-41e6-b74d-abfb047b3598]

comment:12

I rebuilt Sage from the above branch, and tested that it worked, which it did. I've looked at the code changes, which seem appropriate to the issue, doubling the precision until a reduced model can be computed successfully. The changes in gal_reps_number_field are replacing an elliptic curve model with a global integral model of self in two places, which again seems appropriate. The lint and build failures (multiple imports on one line, FeatureNotPresentError) do not seem related to these changes.

Thus - to the extent that I am qualified to do so - I am happy to Ship It!

[davidlowryduda]

comment:13

I agree with Barinder. I'm giving it a positive review.

[davidlowryduda]

comment:14

Whoops, I didn't add myself.

[davidlowryduda]

Reviewer: David Lowry-Duda

[vbraun]

Changed branch from u/cremona/34174 to 783dbc3