search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 482 forks source link

Map to the Weierstrass form #13458

Closed vbraun closed 11 years ago

vbraun commented 12 years ago

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)

Depends on #13084

CC: @novoselt @nbruin @mstreng

Component: algebraic geometry

Author: Volker Braun

Reviewer: Andrey Novoseltsev

Merged: sage-5.12.beta0

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

vbraun commented 12 years ago
comment:1

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

vbraun commented 12 years ago

Dependencies: #13084

vbraun commented 12 years ago

Author: Volker Braun

mstreng commented 11 years ago
comment:2

see also #3416

vbraun commented 11 years ago
comment:3

Rediffed for sage-5.8.beta0

vbraun commented 11 years ago
comment:4

Any takers to review this?

mstreng commented 11 years ago
comment:5

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 commented 11 years ago
comment:6

Rebased for changes to #13084

vbraun commented 11 years ago
comment:7

Rediffed because of changes to #13084

vbraun commented 11 years ago

Updated patch

vbraun commented 11 years ago
comment:8

Attachment: trac_13458_toric_Weierstrass_covering.patch.gz

Rebase had a messed up patch hunk, fixed.

vbraun commented 11 years ago
comment:9

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

novoselt commented 11 years ago
comment:10

I'll try to do it this week.

novoselt commented 11 years ago
comment:11

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 commented 11 years ago

Reviewer: Andrey Novoseltsev

novoselt commented 11 years ago
comment:12

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

novoselt commented 11 years ago
comment:13

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 commented 11 years ago
comment:14

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: E = CPRFanoToricVariety(P).anticanonical_hypersurface()
sage: p = E.defining_polynomials()[0]
sage: WeierstrassForm(p, transformation=True)
vbraun commented 11 years ago
comment:15

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 commented 11 years ago
comment:16

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

novoselt commented 11 years ago
comment:17

Attachment: trac_13458_reviewer.patch.gz

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 commented 11 years ago
comment:18

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 commented 11 years ago
comment:19

Andrey, any more comments?

novoselt commented 11 years ago
comment:20

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!

jdemeyer commented 11 years ago

Merged: sage-5.12.beta0