Opened 10 years ago
Closed 10 years ago
#13100 closed enhancement (fixed)
EllipticCurve_from_j (over QQ) should not always compute minimal twist
Reported by: | John Cremona | Owned by: | John Cremona |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.3 |
Component: | elliptic curves | Keywords: | elliptic curve construction |
Cc: | David Roe, Volker Braun | Merged in: | sage-5.3.beta0 |
Authors: | John Cremona | Reviewers: | David Roe |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #13109 | Stopgaps: |
Description (last modified by )
Currently when constructing an elliptic curve from a j-invariant over QQ, a curve with minimal conductor is created (the so-called "minimal twist"). This could be expensive since it involves factoring j and j-1728, so an option to *not* find the minimal twist should be allowed, with the current behaviour as default for backwards compatibility.
For example:
sage: EllipticCurve(j=2^256+1)
currently triggers factorization of F_8
(which is quite quick) but also F_8-1728
(which is not).
Attachments (3)
Change History (18)
Changed 10 years ago by
Attachment: | trac13100-elliptic.patch added |
---|
comment:1 Changed 10 years ago by
Authors: | → John Cremona |
---|---|
Cc: | David Roe added |
Status: | new → needs_review |
The patch fixes this, and also the issue at #11773 which can be closed as a duplicate.
comment:2 follow-up: 3 Changed 10 years ago by
Status: | needs_review → needs_work |
---|
This looks fine to me. I have no idea what the doctest failing on patchbot is about. Patchbot is also complaining about trailing whitespace....
comment:3 Changed 10 years ago by
Replying to roed:
This looks fine to me. I have no idea what the doctest failing on patchbot is about. Patchbot is also complaining about trailing whitespace....
I removed trailing whitespace on lines I edited (or nearby) but not on the whole file as that makes it harder for reviewers to see what has changed. Of course I could go back and remove the rest. Does patchbot tell us what tests actually fail?
comment:4 follow-up: 5 Changed 10 years ago by
Status: | needs_work → positive_review |
---|
The patchbot will tell you if you click on the yellow disc (or whatever other color it might be), then click on "shortlog."
I ran the tests on my own machine and am not getting the same failure patchbot is. Since I have no idea why your changes would cause a problem in sage.misc.trace, I'm going to give it a positive review.
comment:5 Changed 10 years ago by
Replying to roed:
The patchbot will tell you if you click on the yellow disc (or whatever other color it might be), then click on "shortlog."
I ran the tests on my own machine and am not getting the same failure patchbot is. Since I have no idea why your changes would cause a problem in sage.misc.trace, I'm going to give it a positive review.
Thanks. I agree that the failure reported by patchbot has nothing to do with this ticket.
comment:6 Changed 10 years ago by
Milestone: | sage-5.1 → sage-5.2 |
---|---|
Reviewers: | → David Roe |
comment:7 Changed 10 years ago by
Cc: | Volker Braun added |
---|---|
Dependencies: | → #13109 |
Description: | modified (diff) |
Status: | positive_review → needs_work |
This needs to be rebased to #13109. Patch attached, and I've cc'ed Volker, who should be able to review it quickly.
comment:8 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
comment:9 follow-up: 10 Changed 10 years ago by
John, trac13100-elliptic.patch conflicts with my version of #13109.
comment:10 Changed 10 years ago by
Changed 10 years ago by
Attachment: | trac13100-elliptic-rebased.patch added |
---|
comment:11 Changed 10 years ago by
Description: | modified (diff) |
---|
Sorry, try this one instead. (I got this one confused with another ticket dealing with elliptic curves.)
comment:12 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
This is just a rebasing, so I don't think it needs review.
comment:13 Changed 10 years ago by
Milestone: | sage-5.2 → sage-pending |
---|
comment:14 Changed 10 years ago by
Milestone: | sage-pending → sage-5.3 |
---|
comment:15 Changed 10 years ago by
Merged in: | → sage-5.3.beta0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Applies to 5.1.beta3