Sage: Ticket #13458: Map to the Weierstrass form
https://trac.sagemath.org/ticket/13458
<p>
This module computes the map from a elliptic curve in a toric surface to its Weierstrass form.
</p>
<pre class="wiki"> sage: R.<x,y> = QQ[]
sage: cubic = x^3 + y^3 + 1
sage: WeierstrassMap(cubic)
(-x^3*y^3 - x^3 - y^3,
1/2*x^6*y^3 - 1/2*x^3*y^6 - 1/2*x^6 + 1/2*y^6 + 1/2*x^3 - 1/2*y^3,
x*y)
</pre>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/13458
Trac 1.1.6vbraunThu, 13 Sep 2012 12:15:16 GMTstatus changed; cc, dependencies, author set
https://trac.sagemath.org/ticket/13458#comment:1
https://trac.sagemath.org/ticket/13458#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>cc</strong>
<em>novoselt</em> <em>nbruin</em> added
</li>
<li><strong>dependencies</strong>
set to <em>#13084</em>
</li>
<li><strong>author</strong>
set to <em>Volker Braun</em>
</li>
</ul>
<p>
Nils, since you requested this functionality maybe I can interest you in reviewing this ticket and its dependencies? :-)
</p>
TicketmstrengSun, 02 Dec 2012 21:33:41 GMTcc changed
https://trac.sagemath.org/ticket/13458#comment:2
https://trac.sagemath.org/ticket/13458#comment:2
<ul>
<li><strong>cc</strong>
<em>mstreng</em> added
</li>
</ul>
<p>
see also <a class="closed ticket" href="https://trac.sagemath.org/ticket/3416" title="enhancement: Weierstrass form and Jacobian for cubics and certain other genus-one curves (closed: fixed)">#3416</a>
</p>
TicketvbraunFri, 22 Feb 2013 11:24:09 GMT
https://trac.sagemath.org/ticket/13458#comment:3
https://trac.sagemath.org/ticket/13458#comment:3
<p>
Rediffed for sage-5.8.beta0
</p>
TicketvbraunSat, 18 May 2013 22:43:39 GMT
https://trac.sagemath.org/ticket/13458#comment:4
https://trac.sagemath.org/ticket/13458#comment:4
<p>
Any takers to review this?
</p>
TicketmstrengSun, 19 May 2013 06:04:51 GMT
https://trac.sagemath.org/ticket/13458#comment:5
https://trac.sagemath.org/ticket/13458#comment:5
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13458#comment:4" title="Comment 4">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Any takers to review this?
</p>
</blockquote>
<p>
I don't understand. <a class="closed ticket" href="https://trac.sagemath.org/ticket/13084" title="enhancement: Weierstrass form for toric elliptic curves (closed: fixed)">#13084</a> is listed as a dependency, but needs review. So that needs to happen first.
</p>
TicketvbraunThu, 30 May 2013 14:19:44 GMT
https://trac.sagemath.org/ticket/13458#comment:6
https://trac.sagemath.org/ticket/13458#comment:6
<p>
Rebased for changes to <a class="closed ticket" href="https://trac.sagemath.org/ticket/13084" title="enhancement: Weierstrass form for toric elliptic curves (closed: fixed)">#13084</a>
</p>
TicketvbraunThu, 13 Jun 2013 11:01:48 GMT
https://trac.sagemath.org/ticket/13458#comment:7
https://trac.sagemath.org/ticket/13458#comment:7
<p>
Rediffed because of changes to <a class="closed ticket" href="https://trac.sagemath.org/ticket/13084" title="enhancement: Weierstrass form for toric elliptic curves (closed: fixed)">#13084</a>
</p>
TicketvbraunMon, 17 Jun 2013 21:35:57 GMTattachment set
https://trac.sagemath.org/ticket/13458
https://trac.sagemath.org/ticket/13458
<ul>
<li><strong>attachment</strong>
set to <em>trac_13458_toric_Weierstrass_covering.patch</em>
</li>
</ul>
<p>
Updated patch
</p>
TicketvbraunMon, 17 Jun 2013 21:36:16 GMT
https://trac.sagemath.org/ticket/13458#comment:8
https://trac.sagemath.org/ticket/13458#comment:8
<p>
Rebase had a messed up patch hunk, fixed.
</p>
TicketvbraunTue, 02 Jul 2013 03:37:51 GMT
https://trac.sagemath.org/ticket/13458#comment:9
https://trac.sagemath.org/ticket/13458#comment:9
<p>
This ticket is the last remaining dependency to <a class="closed ticket" href="https://trac.sagemath.org/ticket/3416" title="enhancement: Weierstrass form and Jacobian for cubics and certain other genus-one curves (closed: fixed)">#3416</a> that needs to be reviewed... anyone?
</p>
TicketnovoseltTue, 02 Jul 2013 16:17:09 GMT
https://trac.sagemath.org/ticket/13458#comment:10
https://trac.sagemath.org/ticket/13458#comment:10
<p>
I'll try to do it this week.
</p>
TicketnovoseltWed, 03 Jul 2013 00:47:19 GMTreviewer set
https://trac.sagemath.org/ticket/13458#comment:11
https://trac.sagemath.org/ticket/13458#comment:11
<ul>
<li><strong>reviewer</strong>
set to <em>Andrey Novoseltsev</em>
</li>
</ul>
<p>
Docstrings in <code>sage/geometry/polyhedron/lattice_euclidean_group_element.py</code> are a bit confusing: functions are called dim, one-liner refers to rank, and a note warns that dim is not the same as rank. Can you reword them, Volker? (I've spotted a few other typos in later hunks but fixed them in a reviewer patch.)
</p>
TicketnovoseltWed, 03 Jul 2013 01:03:06 GMT
https://trac.sagemath.org/ticket/13458#comment:12
https://trac.sagemath.org/ticket/13458#comment:12
<p>
Wouldn't it be more natural if <code>transformation=True</code> returned it <em>in addition</em> to the normal form rather than instead of?
</p>
TicketnovoseltWed, 03 Jul 2013 01:13:42 GMT
https://trac.sagemath.org/ticket/13458#comment:13
https://trac.sagemath.org/ticket/13458#comment:13
<p>
When I try
</p>
<pre class="wiki">for P in ReflexivePolytopes(2):
E = CPRFanoToricVariety(P).anticanonical_hypersurface()
p = E.defining_polynomials()[0]
print WeierstrassForm(p, transformation=True)
</pre><p>
it crashes on the 10th polytope with
</p>
<pre class="wiki">Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "_sage_input_2.py", line 10, in <module>
exec compile(u'open("___code___.py","w").write("# -*- coding: utf-8 -*-\\n" + _support_.preparse_worksheet_cell(base64.b64decode("Zm9yIFAgaW4gUmVmbGV4aXZlUG9seXRvcGVzKDIpOgogICAgRSA9IENQUkZhbm9Ub3JpY1ZhcmlldHkoUCkuYW50aWNhbm9uaWNhbF9oeXBlcnN1cmZhY2UoKQogICAgcCA9IEUuZGVmaW5pbmdfcG9seW5vbWlhbHMoKVswXQogICAgcHJpbnQgV2VpZXJzdHJhc3NGb3JtKHAsIHRyYW5zZm9ybWF0aW9uPVRydWUp"),globals())+"\\n"); execfile(os.path.abspath("___code___.py"))
File "", line 1, in <module>
File "/tmp/tmpZDkHaU/___code___.py", line 3, in <module>
exec compile(u'for P in ReflexivePolytopes(_sage_const_2 ):\n E = CPRFanoToricVariety(P).anticanonical_hypersurface()\n p = E.defining_polynomials()[_sage_const_0 ]\n print WeierstrassForm(p, transformation=True)
File "", line 4, in <module>
File "lazy_import.pyx", line 313, in sage.misc.lazy_import.LazyImport.__call__ (sage/misc/lazy_import.c:2475)
File "/home/novoselt/sage-5.11.beta3/local/lib/python2.7/site-packages/sage/schemes/toric/weierstrass.py", line 479, in WeierstrassForm
return WeierstrassMap(polynomial, variables=variables)
File "/home/novoselt/sage-5.11.beta3/local/lib/python2.7/site-packages/sage/schemes/toric/weierstrass_covering.py", line 255, in WeierstrassMap
X,Y,Z = WeierstrassMap_P2(polynomial_aff, variables_aff)
File "/home/novoselt/sage-5.11.beta3/local/lib/python2.7/site-packages/sage/schemes/toric/weierstrass_covering.py", line 339, in WeierstrassMap_P2
H = cubic.Hessian()
File "cachefunc.pyx", line 1722, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (sage/misc/cachefunc.c:9112)
File "/home/novoselt/sage-5.11.beta3/local/lib/python2.7/site-packages/sage/rings/invariant_theory.py", line 1615, in Hessian
return 1/F(216) * H.det()
File "matrix2.pyx", line 1257, in sage.matrix.matrix2.Matrix.det (sage/matrix/matrix2.c:9875)
File "matrix_mpolynomial_dense.pyx", line 584, in sage.matrix.matrix_mpolynomial_dense.Matrix_mpolynomial_dense.determinant (sage/matrix/matrix_mpolynomial_dense.cpp:5877)
File "/home/novoselt/sage-5.11.beta3/local/lib/python2.7/site-packages/sage/rings/polynomial/multi_polynomial_ring.py", line 456, in __call__
return x.sage_poly(self)
File "/home/novoselt/sage-5.11.beta3/local/lib/python2.7/site-packages/sage/interfaces/singular.py", line 1653, in sage_poly
self.name(),variable_str)).split(",")
File "/home/novoselt/sage-5.11.beta3/local/lib/python2.7/site-packages/sage/interfaces/singular.py", line 590, in eval
raise SingularError('Singular error:\n%s'%s)
sage.interfaces.singular.SingularError: Singular error:
? `sage72` is not defined
? error occurred in or before STDIN line 159: `string(coef(sage72,z0*z1*z2*z3));`
? wrong type declaration. type 'help string;'
</pre><p>
Running it just for the 10th is OK, so looks more like a singular interface issue, but may be worth investigation...
</p>
TicketvbraunWed, 03 Jul 2013 02:53:18 GMT
https://trac.sagemath.org/ticket/13458#comment:14
https://trac.sagemath.org/ticket/13458#comment:14
<p>
Why are we even using the Singular interface here, this is pretty sad. I can confirm your bug, even though it works if I just do the 10th polytope without the previous ones
</p>
<pre class="wiki">sage: P = ReflexivePolytopes(2)[10]
sage: E = CPRFanoToricVariety(P).anticanonical_hypersurface()
sage: p = E.defining_polynomials()[0]
sage: WeierstrassForm(p, transformation=True)
</pre>
TicketvbraunWed, 03 Jul 2013 03:00:13 GMT
https://trac.sagemath.org/ticket/13458#comment:15
https://trac.sagemath.org/ticket/13458#comment:15
<p>
PS: Since I wrote the code here I rewrote the matrix groups and added a proper implementation of affine and euclidean groups. This should be used here, so there is no point in embellishing the <code>lattice_euclidean_group_element.py</code> code which is mostly a placeholder.
</p>
TicketvbraunWed, 03 Jul 2013 19:32:42 GMT
https://trac.sagemath.org/ticket/13458#comment:16
https://trac.sagemath.org/ticket/13458#comment:16
<p>
The issue in <a class="ticket" href="https://trac.sagemath.org/ticket/13458#comment:13" title="Comment 13">comment:13</a> (bug in looping over reflexive polygons) is fixed in <a class="closed ticket" href="https://trac.sagemath.org/ticket/14210" title="defect: clean up Matrix_mpolynomial_dense (closed: fixed)">#14210</a>
</p>
TicketnovoseltThu, 04 Jul 2013 22:11:41 GMTattachment set
https://trac.sagemath.org/ticket/13458
https://trac.sagemath.org/ticket/13458
<ul>
<li><strong>attachment</strong>
set to <em>trac_13458_reviewer.patch</em>
</li>
</ul>
TicketnovoseltThu, 04 Jul 2013 22:20:24 GMT
https://trac.sagemath.org/ticket/13458#comment:17
https://trac.sagemath.org/ticket/13458#comment:17
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13458#comment:12" title="Comment 12">novoselt</a>:
</p>
<blockquote class="citation">
<p>
Wouldn't it be more natural if <code>transformation=True</code> returned it <em>in addition</em> to the normal form rather than instead of?
</p>
</blockquote>
<p>
This is still applicable, but otherwise the patch looks good to me modulo some typos fixed in reviewer patch and apparently is works for <a class="closed ticket" href="https://trac.sagemath.org/ticket/3416" title="enhancement: Weierstrass form and Jacobian for cubics and certain other genus-one curves (closed: fixed)">#3416</a> ;-)
</p>
TicketvbraunFri, 05 Jul 2013 00:11:13 GMT
https://trac.sagemath.org/ticket/13458#comment:18
https://trac.sagemath.org/ticket/13458#comment:18
<p>
I thought about whether to return both when <code>transformation=True</code> when I wrote the code originally, but then decided against it. Its not particularly natural the way the computation goes. If speed is an issue (and its at worst a factor 2x for calling <code>WeierstrassForm</code> twice, and in reality its much faster to compute the Weierstrass form without transformation) then you should parametrize the coefficients and derive the formula in one step. So there isn't really any justification.
</p>
<p>
Reviewer patch looks good to me.
</p>
TicketvbraunFri, 12 Jul 2013 13:21:46 GMT
https://trac.sagemath.org/ticket/13458#comment:19
https://trac.sagemath.org/ticket/13458#comment:19
<p>
Andrey, any more comments?
</p>
TicketnovoseltFri, 12 Jul 2013 21:35:34 GMTstatus changed
https://trac.sagemath.org/ticket/13458#comment:20
https://trac.sagemath.org/ticket/13458#comment:20
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Yeap: based on computing transformations for reflexive polygons, it definitely does not seem that speed can be gained by returning both coefficients and transformation, so let it be as it is now. Also, I don't claim to understand all the underlying math involved, but the patch looks reasonable and agrees with Maple package on all 16 polygons (up to appropriate scaling), except that it is WAY faster than Maple. So let's get it in!
</p>
TicketjdemeyerSun, 21 Jul 2013 21:48:33 GMTmilestone changed
https://trac.sagemath.org/ticket/13458#comment:21
https://trac.sagemath.org/ticket/13458#comment:21
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.11</em> to <em>sage-5.12</em>
</li>
</ul>
TicketjdemeyerFri, 02 Aug 2013 14:14:59 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/13458#comment:22
https://trac.sagemath.org/ticket/13458#comment:22
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.12.beta0</em>
</li>
</ul>
Ticket