Opened 9 years ago

Closed 9 years ago

#13100 closed enhancement (fixed)

EllipticCurve_from_j (over QQ) should not always compute minimal twist

Reported by: cremona Owned by: cremona
Priority: minor Milestone: sage-5.3
Component: elliptic curves Keywords: elliptic curve construction
Cc: roed, vbraun Merged in: sage-5.3.beta0
Authors: John Cremona Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13109 Stopgaps:

Status badges

Description (last modified by jhpalmieri)

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

Apply trac13100-elliptic-rebased.patch​.

Attachments (3)

trac13100-elliptic.patch​ (7.4 KB) - added by cremona 9 years ago.
Applies to 5.1.beta3
trac13100-rebase-on-13109.patch​ (1.4 KB) - added by jhpalmieri 9 years ago.
fix a deprecation
trac13100-elliptic-rebased.patch​ (7.7 KB) - added by jhpalmieri 9 years ago.

Download all attachments as: .zip

Change History (18)

Changed 9 years ago by cremona

Applies to 5.1.beta3

comment:1 Changed 9 years ago by cremona

  • Authors set to John Cremona
  • Cc roed added
  • Status changed from new to needs_review

The patch fixes this, and also the issue at #11773 which can be closed as a duplicate.

comment:2 follow-up: Changed 9 years ago by roed

  • Status changed from needs_review to 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 in reply to: ↑ 2 Changed 9 years ago by cremona

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: Changed 9 years ago by roed

  • Status changed from needs_work to 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 in reply to: ↑ 4 Changed 9 years ago by cremona

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 9 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2
  • Reviewers set to David Roe

comment:7 Changed 9 years ago by jhpalmieri

  • Cc vbraun added
  • Dependencies set to #13109
  • Description modified (diff)
  • Status changed from positive_review to 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.

Changed 9 years ago by jhpalmieri

fix a deprecation

comment:8 Changed 9 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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

John, trac13100-elliptic.patch conflicts with my version of #13109.

comment:10 in reply to: ↑ 9 Changed 9 years ago by cremona

Replying to vbraun:

John, trac13100-elliptic.patch conflicts with my version of #13109.

I understand, but doesn't Joh Palmieri's patch fix it (thanks John)?

Changed 9 years ago by jhpalmieri

comment:11 Changed 9 years ago by jhpalmieri

  • Description modified (diff)

Sorry, try this one instead. (I got this one confused with another ticket dealing with elliptic curves.)

comment:12 Changed 9 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

This is just a rebasing, so I don't think it needs review.

comment:13 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.2 to sage-pending

comment:14 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.3

comment:15 Changed 9 years ago by jdemeyer

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