make WeierstrassIsomorphism behave (more) like EllipticCurveIsogeny

[yyyyx4]

This ticket is the obvious next step after #32388: We move lots of code from EllipticCurveIsogeny to EllipticCurveHom, and implement the missing EllipticCurveHom methods in WeierstrassIsomorphism.

This should make WeierstrassIsomorphism close to indistinguishable from EllipticCurveIsogeny.

All the really new code is in weierstrass_morphism.py and should be straightforward. The other changes are just moving things around and some small tweaks that shouldn't cause any observable changes in behaviour.

(The only "real" dependency is #32388. I've merged the others into this branch too because it made sense, but in principle this could be rebased on top of #32388 alone if one of the others gets stalled for some reason.)

Depends on #32388
Depends on #32430
Depends on #32495
Depends on #32490

CC: @defeo @JohnCremona

Component: elliptic curves

Keywords: isogenies, refactoring

Author: Lorenz Panny

Branch/Commit: 444330c

Reviewer: John Cremona

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

[sagetrac-git]

Changed commit from 8414b20 to 440cff0

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

9161ffa	make WeierstrassIsomorphism behave like EllipticCurveIsogeny
440cff0	add test for comparisons between different EllipticCurveHom children

[yyyyx4]

comment:2

See here for a diff without all the dependencies:

• sagemath/sagetrac-mirror@80b0bb11171daf79122859a83d0940bd3fd2423a&id2=8414b20850b468d0b498bab5ac0e2167f97052f5

[sagetrac-git]

Changed commit from 440cff0 to 80b0bb1

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

80b0bb1

reflect this change in the documentation

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

894b145

Merge tag '9.5.beta3' into public/make_WeierstrassIsomorphism_behave_like_EllipticCurveIsogeny

[sagetrac-git]

Changed commit from 80b0bb1 to 894b145

[slel]

comment:5

Sorry, I cannot review in depth; just a few things.

Some .. NOTE:: blocks are indented by only three spaces
instead of the required four. Please fix.

Possible cosmetic changes (feel free to ignore):

-            sage: [E.multiplication_by_m_isogeny(m)(P) == m*P for m in (1,2,3,5,7,9)]
-            [True, True, True, True, True, True]
+            sage: all(E.multiplication_by_m_isogeny(m)(P) == m*P
+            ....:     for m in (1, 2, 3, 5, 7, 9))
+            True

             sage: E = EllipticCurve('99.a1')
             sage: E.multiplication_by_m_isogeny(5)
-            Isogeny of degree 25 from Elliptic Curve defined by y^2 + x*y + y = x^3 - x^2 - 17*x + 30 over Rational Field to Elliptic Curve defined by y^2 + x*y + y = x^3 - x^2 - 17*x + 30 over Rational Field
+            Isogeny of degree 25
+              from Elliptic Curve
+                defined by y^2 + x*y + y = x^3 - x^2 - 17*x + 30
+                over Rational Field
+              to Elliptic Curve
+                defined by y^2 + x*y + y = x^3 - x^2 - 17*x + 30
+                over Rational Field
             sage: E.multiplication_by_m_isogeny(2).rational_maps()
             ((1/4*x^4 + 33/4*x^2 - 121/2*x + 363/4)/(x^3 - 3/4*x^2 - 33/2*x + 121/4),
-             (-1/256*x^7 + 1/128*x^6*y - 7/256*x^6 - 3/256*x^5*y - 105/256*x^5 - 165/256*x^4*y + 1255/256*x^4 + 605/128*x^3*y - 473/64*x^3 - 1815/128*x^2*y - 10527/256*x^2 + 2541/128*x*y + 4477/32*x - 1331/128*y - 30613/256)/(1/16*x^6 - 3/32*x^5 - 519/256*x^4 + 341/64*x^3 + 1815/128*x^2 - 3993/64*x + 14641/256))
+             (-1/256*x^7 + 1/128*x^6*y - 7/256*x^6 - 3/256*x^5*y - 105/256*x^5
+              - 165/256*x^4*y + 1255/256*x^4 + 605/128*x^3*y - 473/64*x^3
+              - 1815/128*x^2*y - 10527/256*x^2 + 2541/128*x*y + 4477/32*x
+              - 1331/128*y - 30613/256)/(1/16*x^6 - 3/32*x^5 - 519/256*x^4
+              + 341/64*x^3 + 1815/128*x^2 - 3993/64*x + 14641/256))

-            sage: [a == b for a in (wE,mE) for b in (wF,mF)]
-            [False, False, False, False]
+            sage: any(a == b for a in (wE, mE) for b in (wF, mF))
+            False

-        #TODO: could have a default implementation that simply
-        #      returns the first component of rational_maps()
+        # TODO: could have a default implementation that simply
+        # returns the first component of rational_maps()

             sage: psi = E1.isogeny(iso.kernel_polynomial(), codomain=E2); psi
-            Isogeny of degree 1 from Elliptic Curve defined by y^2 + 11*x*y + 33*y = x^3 + 22*x^2 + 44*x + 55 over Rational Field to Elliptic Curve defined by y^2 + x*y = x^3 + x^2 - 684*x + 6681 over Rational Field
+            Isogeny of degree 1
+              from Elliptic Curve
+                defined by y^2 + 11*x*y + 33*y = x^3 + 22*x^2 + 44*x + 55
+                over Rational Field
+              to Elliptic Curve
+                defined by y^2 + x*y = x^3 + x^2 - 684*x + 6681
+                over Rational Field

[slel]

Reviewer: Samuel Lelièvre, ...

[yyyyx4]

comment:6

Thanks for having a look! I had just started applying your suggestions, but there are two sources of trouble:

1. Most of the cosmetic changes are actually part of the dependencies and have just been merged.
2. The erroneous indentation appears to be the norm rather than the exception in the elliptic_curves directory: There are almost a thousand lines of code indented with 4k+3 spaces! (The occurrences in the diff itself are not new either; they're the result of moving existing code to a different file.)

To keep things straightforward, my suggestion would be to ignore this here and deal with the problem all at once in a follow-up ticket. I had intended to clean up a few other quirks in the isogenies documentation at some point anyway.

(The intermediate path would be to fix it already in the part of the code that we're touching here and leave the rest for later, but I don't see any real benefit in doing this. Let me know if you think differently.)

[slel]

Changed reviewer from Samuel Lelièvre, ... to none

[slel]

comment:7

You are right, these changes are for a different ticket.
Sorry for the noise.

[sagetrac-git]

Changed commit from 894b145 to 444330c

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

0e58569	Merge tag '9.5.beta4' into public/make_WeierstrassIsomorphism_behave_like_EllipticCurveIsogeny
444330c	Merge tag '9.5.beta5' into public/make_WeierstrassIsomorphism_behave_like_EllipticCurveIsogeny

[mkoeppe]

comment:9

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

[slel]

comment:10

This was essentially reviewed as part of reviewing #32744,
wasn't it? (got that wrong, sorry)

[slel]

Reviewer: John Cremona

[yyyyx4]

comment:12

Thank you!

[vbraun]

Changed branch from public/make_WeierstrassIsomorphism_behave_like_EllipticCurveIsogeny to 444330c