Opened 4 years ago
Last modified 3 years ago
#16115 new enhancement
Class misconception with isogenies
Reported by: | sbesnier | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.4 |
Component: | elliptic curves | Keywords: | days57 |
Cc: | defeo | Merged in: | |
Authors: | Sébastien Besnier | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12880 | Stopgaps: |
Description
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.
Attachments (1)
Change History (11)
Changed 4 years ago by
comment:1 Changed 4 years ago by
- Component changed from algebraic geometry to elliptic curves
- Dependencies set to #12880
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.
comment:2 Changed 4 years ago by
See also #7368 for another to-do list for isogenies.
comment:3 Changed 4 years ago by
- Cc defeo added
comment:4 Changed 4 years ago by
- Keywords days57 added
comment:5 Changed 4 years ago by
comment:6 Changed 4 years ago by
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:
comment:7 Changed 4 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:8 Changed 4 years ago by
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. I don't expect that the work done on this and related tickets will affect that a lot.
comment:9 Changed 4 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:10 Changed 3 years ago by
added
first draft