Compute composite degree (separable) isogenies of EllipticCurves

[grhkm21]

• Implement .isogenies_degree for EllipticCurves

📝 Checklist

• The title is concise, informative, and self-explanatory.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

⌛ Dependencies

[yyyyx4]

Other than the few small remarks, looks good to me!

[grhkm21]

Thanks for the review :)

[yyyyx4]

In addition to the smaller code remarks above below, I'm also wondering if this should be an iterator: The list can grow very quickly, so doing it as a depth-first iterator might be a good idea?

[grhkm21]

I swear I wrote some code similar to the previous two commits, but somehow didn't commit it, and it's not in my git stash either. Anyways, please kindly review it once again :) it's very short now.

[github-actions]

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

[yyyyx4]

There are duplicates in the list as soon as any prime appears with multiplicity 3 or more in the degree:

sage: E = EllipticCurve(GF(11^2), [1,0])
sage: len(list(E.isogenies_degree(2^3)))
27
sage: len(set(E.isogenies_degree(2^3)))
15

To fix it, we could restrict to cyclic isogenies:

sage: P,Q = E.torsion_basis(2)
sage: len([phi for phi in E.isogenies_degree(2^3) if phi(P) or phi(Q)])
12
sage: len({phi for phi in E.isogenies_degree(2^3) if phi(P) or phi(Q)})
12

Returning only cyclic isogenies is arguably "better" anyway, since non-cyclic isogenies could be considered somewhat degenerate among the set of all isogenies of given degree. However, adding this check will make the code quite a bit more complicated. I'm unsure how to proceed.

[grhkm21]

Oh right, sorry for missing that. We can have a check to ensure we are not taking the dual isogeny of the previous taken one, since the prime degrees considered are sorted.

[hellman]

I think it would be useful to have an phi1.is_dual_of(phi2) method (up to isomorphism check). I also need something like for mitm.

But if we add it to the EllipticCurveHom class, it should be generic (e.g. factor the degree?), which could introduce an overhead.

[grhkm21]

hi. i am back on this pr

[grhkm21]

Btw one problem I'm trying to fix in the past few commits is that the only way the new compare_via_evaluation calling _has_order_at_least can return False right now is if the ._order (cached order) is computed already, but that's not the case for compare_via_evaluation. As a result, if a point has finite order, your _has_order_at_least will use all $999$ attempts to prove that it has finite order. Therefore, we want to use the information about n.

One method I'm trying is to keep track of P * n as n goes, but over number fields the coefficients go crazy large, even though the polynomial degree is bounded by the number field degree.

[grhkm21]

Even simple optimisations like all(not Q.is_zero() for Q in multiples(P, bound)) or something when bound is small just becomes infeasible when the number field degree is say > 10?

[grhkm21]

I have this patch, but I'll experiment more with it first. Back later..

diff --git a/src/sage/schemes/elliptic_curves/ell_point.py b/src/sage/schemes/elliptic_curves/ell_point.py
index 77d9f6e981d..1b8859f166a 100755
--- a/src/sage/schemes/elliptic_curves/ell_point.py
+++ b/src/sage/schemes/elliptic_curves/ell_point.py
@@ -121,6 +121,7 @@ import sage.groups.generic as generic
 import sage.rings.abc
 
 from sage.misc.lazy_import import lazy_import
+from sage.groups.generic import order_from_multiple
 from sage.rings.finite_rings.integer_mod import Mod
 from sage.rings.infinity import Infinity as oo
 from sage.rings.integer import Integer
@@ -134,6 +135,7 @@ from sage.schemes.projective.projective_point import (SchemeMorphism_point_proje
                                                       SchemeMorphism_point_abelian_variety_field)
 from sage.structure.coerce_actions import IntegerMulAction
 from sage.structure.element import AdditiveGroupElement
+from sage.structure.factorization import Factorization
 from sage.structure.richcmp import richcmp
 from sage.structure.sequence import Sequence
 
@@ -2599,7 +2601,6 @@ class EllipticCurvePoint_number_field(EllipticCurvePoint_field):
         from sage.sets.primes import Primes
         from sage.rings.finite_rings.finite_field_constructor import GF
         field_deg = self.curve().base_field().absolute_degree()
-        print("field_deg:", field_deg)
         if field_deg > 1:
             K = self.curve().base_field().absolute_field('T')
             _, iso = K.structure()
@@ -2616,11 +2617,8 @@ class EllipticCurvePoint_number_field(EllipticCurvePoint_field):
             bound = min(bound, 12 + 1)
         assert P.curve() is E
 
-        if bound <= 20:
-            # fast path for testing small orders
-            return not any((P * i).is_zero() for i in range(1, bound))
-
-        n = ZZ.one()
+        n = Factorization([])
+        Pmul = P
         no_progress = 0
         for p in Primes():
             try:
@@ -2636,18 +2634,23 @@ class EllipticCurvePoint_number_field(EllipticCurvePoint_field):
             except (ZeroDivisionError, ArithmeticError):
                 continue
 
-            o = Pred.order()
-            if not o.divides(n):
-                n = n.lcm(o)
+            o = Pred.order().factor()
+            if not o.value().divides(n.value()):
+                g = n.gcd(o)
+                n *= g
+                if n.value() >= bound:
+                    return True
+
+                Pmul *= g.value()
+                if Pmul.is_zero():
+                    self.set_order(n.value())
+                    return n.value() >= bound
                 no_progress = 0
             else:
                 no_progress += 1
 
-            if n >= bound:
-                return True
             if no_progress >= attempts:
                 return
-            print("n:", n)
 
         assert False  # unreachable unless there are only finitely many primes

I'm quite certain Pmul *= g.value() would be very slow when the point order is infinite, so maybe we should defer that multiplication to when no_progress is going on for a long time.

[yyyyx4]

Just to clarify, is the version I had still too slow for the tests here? If it isn't, I'd suggest merging this soon, and other speedups for comparing isogenies over number fields could still be added in a later patch.

[grhkm21]

Hey, thanks for the reminder. Actually yes your version is fast enough in many cases. I will remove my debug output and let's get this merged. I will continue thinking about faster number field isogeny comparisons in another PR.

[grhkm21]

tested the problematic files locally and seems fine.

[yyyyx4]

Tiny (optional) suggestion: Rename .isogenies_degree() to .isogenies_of_degree() for grammar?

[grhkm21]

Hmm, then we shouold change to isogenies_of_prime_degree as well. As a user if I found out about isogenies_prime_degree but I don't want prime then I'll probably try isogenies_degree 🤔

[yyyyx4]

I'd argue that .isogenies_prime_degree() could eventually become internal, even. It is a special case of this more general method after all.

[grhkm21]

Oh, that is true. That'll be a year down the line though

[S17A05]

Just one more small thing (plus the name change suggested by @yyyyx4, in case you want to implement it now), then I will approve

[yyyyx4]

Needs rebase, apparently.

[grhkm21]

Thanks! Done. Fun fact, the latest commit hash starts with 73917522060. 11 digits! (0.5%)

[yyyyx4]

Thanks! Let's finally get this merged. 🥳