New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
insufficient precision in scaling elliptic curves over number fields by units #34174
Comments
comment:1
I'm sure it's because the fundamental unit is very skew:
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:
It should be possible to do |
comment:2
The For the second issue, line 1523-6 of
and the last line can raise an error when The first (main) issue is in
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. |
Author: John Cremona |
Commit: |
This comment has been minimized.
This comment has been minimized.
Branch: u/cremona/34174 |
comment:3
Both issues are fixed in branch u/cremona/34174, with two doctests added. New commits:
|
This comment has been minimized.
This comment has been minimized.
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 |
Changed author from John Cremona to none |
Changed branch from u/cremona/34174 to none |
Changed commit from |
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 |
Commit: |
Branch: u/cremona/34174 |
Author: John Cremona |
This comment has been minimized.
This comment has been minimized.
comment:8
Replying to @BarinderBanwait:
It's OK, you don't need to revert anything. |
comment:9
Replying to @JohnCremona:
--but it would be good if you were able to review this to say that the problem is fixed! |
comment:10
Replying to @JohnCremona:
Sure! I'll checkout your branch, rebuild sage, and test that it now works |
comment:11
Replying to @BarinderBanwait:
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). |
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 Thus - to the extent that I am qualified to do so - I am happy to Ship It! |
comment:13
I agree with Barinder. I'm giving it a positive review. |
comment:14
Whoops, I didn't add myself. |
Reviewer: David Lowry-Duda |
Changed branch from u/cremona/34174 to |
<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:
This raises a
KeyError
in the first instance, which seems to be handled via a direct call toIsogenyClass_EC_NumberField
, but this then raises aValueError: 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
isLinux 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 onLinux 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
The text was updated successfully, but these errors were encountered: