EllipticCurve_from_j (over QQ) should not always compute minimal twist

[JohnCremona]

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 attachment: trac13100-elliptic-rebased.patch.

Depends on #13109

CC: @roed314 @vbraun

Component: elliptic curves

Keywords: elliptic curve construction

Author: John Cremona

Reviewer: David Roe

Merged: sage-5.3.beta0

Issue created by migration from https://trac.sagemath.org/ticket/13100

[JohnCremona]

Applies to 5.1.beta3

[JohnCremona]

Author: John Cremona

[JohnCremona]

comment:1

Attachment: trac13100-elliptic.patch.gz

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

[roed314]

comment:2

This looks fine to me. I have no idea what the doctest failing on patchbot is about. Patchbot is also complaining about trailing whitespace....

[JohnCremona]

comment:3

Replying to @roed314:

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?

[roed314]

comment:4

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.

[JohnCremona]

comment:5

Replying to @roed314:

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.

[jdemeyer]

Reviewer: David Roe

[jhpalmieri]

Dependencies: #13109

[jhpalmieri]

comment:7

This needs to be rebased to #13109. Patch attached, and I've cc'ed Volker, who should be able to review it quickly.

[jhpalmieri]

fix a deprecation

[jhpalmieri]

comment:8

Attachment: trac13100-rebase-on-13109.patch.gz

[vbraun]

comment:9

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

[JohnCremona]

comment:10

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

[jhpalmieri]

Attachment: trac13100-elliptic-rebased.patch.gz

[jhpalmieri]

comment:11

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

[jhpalmieri]

comment:12

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

[jdemeyer]

Merged: sage-5.3.beta0