sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.46k stars 482 forks source link

Isogeny small degree improvement #38995

Open user202729 opened 4 days ago

user202729 commented 4 days ago

Allow the following to be called without error.

sage: EllipticCurve(GF(31).algebraic_closure(), [0, 6, 0, 1, 0]).isogenies_prime_degree(5)

Inspired from, but independent from, https://github.com/sagemath/sage/issues/38373 . Whatever consensus on that issue might be.


Would be good if someone can sanity check that the output is actually correct. (although it looks correct to me.)

Strangely enough there is

        if isinstance(F, sage.rings.abc.AlgebraicField):
            raise NotImplementedError("This code could be implemented for QQbar, but has not been yet.")

even though when I comment that part out, the code seems to work well.

:memo: Checklist

:hourglass: Dependencies

https://github.com/sagemath/sage/pull/38994 : both changes are needed to make it work.

github-actions[bot] commented 4 days ago

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

JohnCremona commented 3 days ago

About correctness: looks reasonable to me. Over GF(31) there are only 2 5-isogenies, the others are defined over GF(31^2) and the output from that looks just like this output. Perhaps it would be safer to have the doctest just report that there are six 5-isogenies, to guard against future versions making different choices? My familiarity with the supersingular case is less than perfect! I note that of the 6 isogenous curves there are only 3 isomorphism classes, E itself (@3) another @2 and another unique.

I think it is great that this works as it should making only a trivial change to the code in isogenies_small_degree which I wrote years ago.

Can you explain the need for the new copy methods?

user202729 commented 3 days ago

The copy is from https://github.com/sagemath/sage/pull/38994 . Basically it's needed to make it work at all because of some implementation detail.

Specifically, some part of the code requires making a new isogeny with a different pre-composed isomorphism, so it calls the internal method _set_pre_isomorphism (which mutates the isogeny (!)), but it wants a modified copy so it does deepcopy on the isogeny. Without special handling, deepcopy on the isogeny copies all the members, including the base field.

I think it's desirable for arbitrary objects to be copyable anyway, so I can't see any harm. (Or at least if it's desirable to make algebraic closure of finite field not copyable then at least raise a meaningful error, but at the moment loads(dumps(GF(p).algebraic_closure())) makes a copy anyway)

It might separately be a good idea to refactor the code to avoid mutable state in isogeny objects (easy to introduce accidental bugs), but currently it's just internal implementation detail anyway.