New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Map to the Weierstrass form #13458
Comments
comment:1
Nils, since you requested this functionality maybe I can interest you in reviewing this ticket and its dependencies? :-) |
Dependencies: #13084 |
Author: Volker Braun |
comment:2
see also #3416 |
comment:3
Rediffed for sage-5.8.beta0 |
comment:4
Any takers to review this? |
comment:6
Rebased for changes to #13084 |
comment:7
Rediffed because of changes to #13084 |
Updated patch |
comment:8
Attachment: trac_13458_toric_Weierstrass_covering.patch.gz Rebase had a messed up patch hunk, fixed. |
comment:9
This ticket is the last remaining dependency to #3416 that needs to be reviewed... anyone? |
comment:10
I'll try to do it this week. |
comment:11
Docstrings in |
Reviewer: Andrey Novoseltsev |
comment:12
Wouldn't it be more natural if |
comment:13
When I try
it crashes on the 10th polytope with
Running it just for the 10th is OK, so looks more like a singular interface issue, but may be worth investigation... |
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
|
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 |
comment:16
The issue in comment:13 (bug in looping over reflexive polygons) is fixed in #14210 |
comment:17
Attachment: trac_13458_reviewer.patch.gz Replying to @novoselt:
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
I thought about whether to return both when Reviewer patch looks good to me. |
comment:19
Andrey, any more comments? |
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! |
Merged: sage-5.12.beta0 |
This module computes the map from a elliptic curve in a toric surface to its Weierstrass form.
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
The text was updated successfully, but these errors were encountered: