Map to the Weierstrass form

[vbraun]

This module computes the map from a elliptic curve in a toric surface to its Weierstrass form.

    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)

[vbraun]

Nils, since you requested this functionality maybe I can interest you in reviewing this ticket and its dependencies? :-)

[mstreng]

see also #3416

[vbraun]

Rediffed for sage-5.8.beta0

[vbraun]

Any takers to review this?

[mstreng]

Replying to vbraun:

Any takers to review this?

I don't understand. #13084 is listed as a dependency, but needs review. So that needs to happen first.

[vbraun]

Rebased for changes to #13084

[vbraun]

Rediffed because of changes to #13084

[vbraun]

Updated patch

[vbraun]

Rebase had a messed up patch hunk, fixed.

[vbraun]

This ticket is the last remaining dependency to #3416 that needs to be reviewed... anyone?

[novoselt]

I'll try to do it this week.

[novoselt]

Docstrings in sage/geometry/polyhedron/lattice_euclidean_group_element.py 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.)

[novoselt]

Wouldn't it be more natural if transformation=True returned it in addition to the normal form rather than instead of?

[novoselt]

When I try

for P in ReflexivePolytopes(2):
    E = CPRFanoToricVariety(P).anticanonical_hypersurface()
    p = E.defining_polynomials()[0]
    print WeierstrassForm(p, transformation=True)

it crashes on the 10th polytope with

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;'

Running it just for the 10th is OK, so looks more like a singular interface issue, but may be worth investigation...

[vbraun]

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

sage: P = ReflexivePolytopes(2)[10]
sage: P = ReflexivePolytopes(2)[10]
sage: E = CPRFanoToricVariety(P).anticanonical_hypersurface()
sage: p = E.defining_polynomials()[0]
sage: WeierstrassForm(p, transformation=True)

[vbraun]

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 lattice_euclidean_group_element.py code which is mostly a placeholder.

[vbraun]

The issue in comment:13 (bug in looping over reflexive polygons) is fixed in #14210

[novoselt]

Replying to novoselt:

Wouldn't it be more natural if transformation=True returned it in addition to the normal form rather than instead of?

This is still applicable, but otherwise the patch looks good to me modulo some typos fixed in reviewer patch and apparently is works for #3416 ;-)

[vbraun]

I thought about whether to return both when transformation=True 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 WeierstrassForm 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.

Reviewer patch looks good to me.

[vbraun]

Andrey, any more comments?

[novoselt]

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!