-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
Vélu isogeny formulas use incorrect a-invariants when pre-isomorphism is set #33214
Comments
Author: Lorenz Panny |
Commit: |
comment:2
looks good to me, and simple enough John or Luca, do you approve ? |
comment:3
Well spotted, sounds very reasonable. Can you add something to the doctest which makes it clear that this is correct in the example, while the same code pre-patch is not? Some assertion which would fail before? Perhaps composing with a dual and checking that the composite multiplies by the degree? That way people can see that this is correct, without having to do some hard computations to verify. |
comment:5
Thanks! I just added another test to verify that the rational maps of the isogeny with pre-isomorphism satisfy the (co)domain curve equations. (The rational maps are internally computed by evaluating the isogeny at a generic point, i.e., it uses the same code path that is corrected here.) Both tests fail with a current Sage and pass after this patch. |
Reviewer: John Cremona, Frédéric Chapoton |
comment:6
Replying to @yyyyx4:
That's great, just what I was suggesting. |
Changed branch from public/fix_velu_isogeny_evaluation_with_pre_isomorphism to |
With Sage 9.5.rc3:
The reason is the following code in
EllipticCurveIsogeny.__compute_via_velu()
:Here,
.__E1
is the same as.domain()
, but the Vélu formulas compute the "inner" isogeny. Thus, these should be thea1,a3
constants of the pre-isomorphism codomain.CC: @JohnCremona @categorie @defeo
Component: elliptic curves
Author: Lorenz Panny
Branch/Commit:
3b823d0
Reviewer: John Cremona, Frédéric Chapoton
Issue created by migration from https://trac.sagemath.org/ticket/33214
The text was updated successfully, but these errors were encountered: