Opened 9 years ago
Closed 5 years ago
#11327 closed defect (fixed)
isogeny code uses deprecated(?) multivariate gcd
Reported by:  cjh  Owned by:  cremona 

Priority:  major  Milestone:  sage6.4 
Component:  elliptic curves  Keywords:  isogeny multivariate dual 
Cc:  Merged in:  
Authors:  John Cremona  Reviewers:  Chris Wuthrich 
Report Upstream:  N/A  Work issues:  
Branch:  4abaea8 (Commits)  Commit:  4abaea8b85879a9fbe91404e8a43b57f994938de 
Dependencies:  Stopgaps: 
Description (last modified by )
New branch sorts out the use of uni versus multivariate polynomials systematically, and makes a lot of minor improvements and simplifications and improves many docstrings. The specific errors originally reported here (see below) and at #16779 are fixed.
In sage.schemes.elliptic_curves.ell_curve_isogeny (of sage 4.6.2) the routine two_torsion_part() invokes a gcd on two polynomials. When the polynomials belong to a multivariate ring, an error occurs. Perhaps this is because the gcd routine in sage.rings.polynomial.multi_polynomial_element has been commented out (because it uses Singular?!).
Converting psi (and perhaps psi2) to univariate polynomials within two_torsion_part() before taking the gcd seems to work, but there seems to be other code in the module which uses a multivariate ring when a univariate ring would suffice (e.g. in init_from_kernel_polynomial).
I discovered this error when trying to create a dual isogeny (for an elliptic curve over Q(t)). The original isogeny was a cyclic 2isogeny and thus the dual should also be a cyclic 2isogeny. Without giving more details on how I created the curve, let me simply report the error message which results when I call isogeny.dual():
Traceback (most recent call last):
File "<stdin>", line 1, in <module> File "_sage_input_5.py", line 10, in <module>
exec compile(u'open("_code_.py","w").write("# * coding: utf8 *
n" + _support_.preparse_worksheet_cell(base64.b64decode("cHNpID0gcGhpLmR1YWwoKQ=="),globals())+"
n"); execfile(os.path.abspath("_code_.py"))File "", line 1, in <module>
File "/private/var/folders/4V/4VAOdForFpanThca5kJ5xE+++TI/Tmp/tmp8DXIBC/_code_.py", line 2, in <module>
exec compile(u'psi = phi.dual()
File "", line 1, in <module>
File "/Applications/sage/local/lib/python2.6/sitepackages/sage/schemes/elliptic_curves/ell_curve_isogeny.py", line 3248, in dual
phi_hat = EllipticCurveIsogeny?(E1, None, E2, d)
File "/Applications/sage/local/lib/python2.6/sitepackages/sage/schemes/elliptic_curves/ell_curve_isogeny.py", line 914, in init
self.init_from_kernel_polynomial(kernel, degree)
File "/Applications/sage/local/lib/python2.6/sitepackages/sage/schemes/elliptic_curves/ell_curve_isogeny.py", line 2038, in init_from_kernel_polynomial
psi_G = two_torsion_part(E, poly_ring, psi, degree);
File "/Applications/sage/local/lib/python2.6/sitepackages/sage/schemes/elliptic_curves/ell_curve_isogeny.py", line 453, in two_torsion_part
psi_G = poly_ring(psi.gcd(psi_2))
File "element.pyx", line 327, in sage.structure.element.Element.getattr (sage/structure/element.c:2715) File "parent.pyx", line 277, in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2841) File "parent.pyx", line 177, in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2663)
AttributeError?: 'MPolynomial_polydict' object has no attribute 'gcd'
Attachments (1)
Change History (13)
comment:1 Changed 9 years ago by
comment:2 Changed 8 years ago by
 Milestone changed from sage4.7.1 to sage4.7.2
The patch trac_11327isogs.patch was made by me from an edited version of ell_curve_isogeny.py which cjh sent me. It needs work.
comment:3 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:4 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:5 Changed 6 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:6 Changed 5 years ago by
 Dependencies set to #16779
comment:7 Changed 5 years ago by
 Branch set to u/cremona/ticket/11327
 Commit set to e043ae8eeb0ed3b12f748131904d5e0e0476a83e
 Status changed from new to needs_review
New commits:
e043ae8  #11327,#16779: improvements to isogeny class internals and docstrings

comment:8 Changed 5 years ago by
 Dependencies #16779 deleted
 Description modified (diff)
comment:9 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:10 Changed 5 years ago by
 Branch changed from u/cremona/ticket/11327 to u/wuthrich/ticket/11327
 Commit changed from e043ae8eeb0ed3b12f748131904d5e0e0476a83e to 4abaea8b85879a9fbe91404e8a43b57f994938de
 Reviewers set to Chris Wuthrich
 Status changed from needs_review to positive_review
All tests pass and the problem is solved indeed. Thanks also for all the improvments on the documentation.
Tiny: There is a "#" appearing in the documentation of the class EllipticCurveIsogeny
. So I changed that.
New commits:
4abaea8  trac 16779: small correction in doc

comment:11 Changed 5 years ago by
Thanks a lot!
comment:12 Changed 5 years ago by
 Branch changed from u/wuthrich/ticket/11327 to 4abaea8b85879a9fbe91404e8a43b57f994938de
 Resolution set to fixed
 Status changed from positive_review to closed
I have run into this myself, but never had time to fix it as the problem always seemed to grow bigger. Feel free to make a patch! Before you do, please note that there are other patches around which affect this file  one of which splits it into two  so it would best to work on top of that.