Class misconception with isogenies

[sagetrac-sbesnier]

The isogenies implementation suffers several conception issues; the main issues I can quickly describe are:

• the _mul_ method of EllipticCurveIsogeny is not handled by the general Morphism class:

sage: E = EllipticCurve(j=GF(7)(0))
sage: phi=E.isogeny( [E(0), E((0,1)), E((0,-1))])
sage: phi*phi
[...]
TypeError: Elliptic Curve defined by y^2 = x^3 + 1 over Finite Field of size 7 is not in Category of hom sets in Category of Schemes

--> I've edited ell_curve_isogeny.py in order to make this work: the codomain() and domain() methods initially gaves "Elliptic Curve ...", I've updated to "Abelian group of points of Elliptic Curve ..." (and tried to make the other parts of code consistant with that).

• Hierarchy class is not well think: e.g. WeierstrassIsomorphism is not a child of EllipticCurveIsogeny. Moreover, we can not compose isogeny and Weierstrass Isomorphism. There exist a "SchemeMorphism" class; maybe it's a good idea to make EllipticCurveIsogeny subclass of this ?
  Follwing the previous code:

sage: i=WeierstrassIsomorphism(E,(2,0,0,0))
sage: phi*i
Composite map:
  From: Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 1 over Finite Field of size 7
  To:   Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 1 over Finite Field of size 7
  Defn:   Generic morphism:
          From: Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 1 over Finite Field of size 7
          To:   Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 1 over Finite Field of size 7
          Via:  (u,r,s,t) = (2, 0, 0, 0)
        then
          Isogeny of degree 3:
          From: Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 1 over Finite Field of size 7
          To: Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 1 over Finite Field of size 7
sage: i*phi
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-13-b2d0bfcb3b82> in <module>()
----> 1 i*phi

/home/sebsheep/sage/local/lib/python2.7/site-packages/sage/schemes/elliptic_curves/weierstrass_morphism.pyc in __mul__(self, other)
    582             True
    583         """
--> 584         if self._domain_curve==other._codomain_curve:
    585             w=baseWI.__mul__(self,other)
    586             return WeierstrassIsomorphism(other._domain_curve, w.tuple(), self._codomain_curve)

/home/sebsheep/sage/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (sage/structure/element.c:3868)()

/home/sebsheep/sage/local/lib/python2.7/site-packages/sage/structure/misc.so in sage.structure.misc.getattr_from_other_class (sage/structure/misc.c:1696)()

AttributeError: 'EllipticCurveIsogeny' object has no attribute '_codomain_curve'

I've joined a "preliminary draft" of a new "sage/local/lib/python2.7/site-packages/sage/schemes/elliptic_curves/ell_curve_isogeny.py" wrote in a few hours.

Depends on #12880

CC: @defeo @yyyyx4

Component: elliptic curves

Keywords: days57

Reviewer: Matthias Koeppe

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

[sagetrac-sbesnier]

first draft

[pjbruin]

Dependencies: #12880

[pjbruin]

comment:1

Attachment: ell_curve_isogeny.py.gz

There are indeed various aspects in which isogenies could be improved.

As for your first point, this should be addressed in #12880. (Note that the mathematically correct solution is to make the domain and codomain be the elliptic curves themselves, not the groups of points.) This can only really be fixed after #11474.

Tying the various classes closer together as in your second point is certainly worth looking at. One could even consider making EllipticCurveIsogeny inherit from SchemeMorphism_polynomial_projective_space, but perhaps that class is too specialised in an orthogonal direction.

It is not recommended to simply upload the Python files you changed; in that way it is impossible to see what your changes are or on what version of Sage you based them. Use Git instead; see http://sagemath.org/doc/developer/ for an introduction to the Sage development process.

Finally, please be aware that "misconception" is not the most charitable way to describe the work that has been done so far. A lot of code in Sage dates from years back when the "right" infrastructure (more specific base classes, category code etc.) was simply not available. There is certainly a lot of room to improve the design in many parts of Sage, but time and resources are limited, and changes need to be made in small steps to avoid breaking existing code.

[pjbruin]

comment:2

See also #7368 for another to-do list for isogenies.

[defeo]

Changed keywords from none to days57

[defeo]

Changed author from sbesnier to Sébastien Besnier

[jpflori]

comment:6

About SchemeMorphism also see the description of #14711 of why it currently does not inherit Morphism (simple enough reason, cython won't let you do that currently...).

IIRC this was the corresponding discussion:

• https://groups.google.com/d/msg/cython-users/-GvwK6sEF0E/vAaliNBQ9XMJ

[JohnCremona]

comment:8

I implemented Weierstrass morphisms in 2007: it was the first piece of Sage development I did. Until that time it was impossible to change models or move points from one model to another! And the implementation of isogenies was done much later than that.

I do agree that the EllipticCurveIsogeny class is not designed in the way I would want it to be, but it works well enough and has been used an enormous amount over QQ and over number fields, even if it has not been possible (for example) to compose isogenies.

I also agree with Peter that the domain and codomain must be elliptic curves, and not sets or groups of points.

I am currently working on extending the class IsogenyClass_EC to work over number fields (see #16743). I don't expect that the work done on this and related tickets will affect that a lot.

[sagetrac-adhalanay]

comment:10

added

[yyyyx4]

comment:12

I think this is resolved with #32388 and #32502.

[mkoeppe]

comment:13

I agree.

[mkoeppe]

Reviewer: Matthias Koeppe

[mkoeppe]

Changed author from Sébastien Besnier to none