Opened 9 years ago

Closed 8 years ago

#13458 closed enhancement (fixed)

Map to the Weierstrass form

Reported by: vbraun Owned by: AlexGhitza
Priority: major Milestone: sage-5.12
Component: algebraic geometry Keywords:
Cc: novoselt, nbruin, mstreng Merged in: sage-5.12.beta0
Authors: Volker Braun Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13084 Stopgaps:

Status badges

Description

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)

Attachments (2)

trac_13458_toric_Weierstrass_covering.patch (25.2 KB) - added by vbraun 8 years ago.
Updated patch
trac_13458_reviewer.patch (6.0 KB) - added by novoselt 8 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 9 years ago by vbraun

  • Authors set to Volker Braun
  • Cc novoselt nbruin added
  • Dependencies set to #13084
  • Status changed from new to needs_review

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

comment:2 Changed 9 years ago by mstreng

  • Cc mstreng added

see also #3416

comment:3 Changed 9 years ago by vbraun

Rediffed for sage-5.8.beta0

comment:4 follow-up: Changed 9 years ago by vbraun

Any takers to review this?

comment:5 in reply to: ↑ 4 Changed 9 years ago by 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.

comment:6 Changed 9 years ago by vbraun

Rebased for changes to #13084

comment:7 Changed 8 years ago by vbraun

Rediffed because of changes to #13084

Changed 8 years ago by vbraun

Updated patch

comment:8 Changed 8 years ago by vbraun

Rebase had a messed up patch hunk, fixed.

comment:9 Changed 8 years ago by vbraun

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

comment:10 Changed 8 years ago by novoselt

I'll try to do it this week.

comment:11 Changed 8 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev

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.)

comment:12 follow-up: Changed 8 years ago by novoselt

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

comment:13 Changed 8 years ago by 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...

comment:14 Changed 8 years ago by 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: E = CPRFanoToricVariety(P).anticanonical_hypersurface()
sage: p = E.defining_polynomials()[0]
sage: WeierstrassForm(p, transformation=True)
Last edited 8 years ago by vbraun (previous) (diff)

comment:15 Changed 8 years ago by 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.

comment:16 Changed 8 years ago by vbraun

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

Changed 8 years ago by novoselt

comment:17 in reply to: ↑ 12 Changed 8 years ago by 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 ;-)

comment:18 Changed 8 years ago by 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.

comment:19 Changed 8 years ago by vbraun

Andrey, any more comments?

comment:20 Changed 8 years ago by novoselt

  • Status changed from needs_review to positive_review

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!

comment:21 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:22 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.