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: |
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)
Change History (24)
comment:1 Changed 9 years ago by
- Cc novoselt nbruin added
- Dependencies set to #13084
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
Rediffed for sage-5.8.beta0
comment:4 follow-up: ↓ 5 Changed 8 years ago by
Any takers to review this?
comment:5 in reply to: ↑ 4 Changed 8 years ago by
comment:6 Changed 8 years ago by
Rebased for changes to #13084
comment:7 Changed 8 years ago by
Rediffed because of changes to #13084
comment:8 Changed 8 years ago by
Rebase had a messed up patch hunk, fixed.
comment:9 Changed 8 years ago by
This ticket is the last remaining dependency to #3416 that needs to be reviewed... anyone?
comment:10 Changed 8 years ago by
I'll try to do it this week.
comment:11 Changed 8 years ago by
- 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: ↓ 17 Changed 8 years ago by
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
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
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)
comment:15 Changed 8 years ago by
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
The issue in comment:13 (bug in looping over reflexive polygons) is fixed in #14210
Changed 8 years ago by
comment:17 in reply to: ↑ 12 Changed 8 years ago by
comment:18 Changed 8 years ago by
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
Andrey, any more comments?
comment:20 Changed 8 years ago by
- 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
- Milestone changed from sage-5.11 to sage-5.12
comment:22 Changed 8 years ago by
- Merged in set to sage-5.12.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
Nils, since you requested this functionality maybe I can interest you in reviewing this ticket and its dependencies? :-)