Opened 3 years ago

Closed 19 months ago

#14060 closed defect (fixed)

Reported by: Owned by: cremona cremona major sage-6.2 elliptic curves pari precision wuthrich John Cremona Chris Wuthrich N/A u/wuthrich/ticket/14060 (Commits) 634ab955474c1d1ed637171a96a85e3d26f3f5e0

We need a better implementation of minimal_quadratic_twist(), for example if the conductor is square-free then there is no need to do so much work as is currently done. Additionally, when j(E)=0 or 1728 the curve returned is a minimal quadratic twist, but not necessarily the minimal twist, and it would be a good thing for the documentation to make this clear.

comment:1 Changed 2 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 20 months ago by jdemeyer

• Description modified (diff)
• Summary changed from Some elliptic curve functions fail since pari_curve is created with insufficient precision to Improve minimal_quadratic_twist()

Precision issue is #13163.

comment:3 Changed 20 months ago by cremona

I see two possibilities here: either rename this function minimal_twist() and return (as it does now) a curve which may be a sextic or quartic twist when j=0 or j=1728; or keep this name and be more restrictive for those j-invariants so that the returned curve literally is a quadratic twist. A third possibility is to have both named functions, doing the same thing unless j=0 or j=1728.

I am CC-ing Chris W since the only place where this function is used (according to search_src()) is in sha_tate.py, line 540, and whatever we do must be compatible with what is needed there!

Of course this is in addition to adding one line to return self of self.conductor().is_squarefree().

comment:4 Changed 20 months ago by wuthrich

In sha_tate and in general for p-adic L-functions we can only twist by quadratic characters (well, I can only, maybe someone else knows more). So if changing the function one has to be careful to twist only by quadratic twists for these two cm curves. But if I remember correctly, we currently exclude cm curve (which we would not have to do in fact).

comment:5 Changed 20 months ago by cremona

Looking more carefully at the code (I am not embarrassed that I wrote it but could not remember) I see that for j=0 and j=1728 the curve returned is always a quadratic twist. So the "additionally" part of the ticket's current description is incorrect.

If you agree with me on that (and a second opinion would be welcome!) then that just leaves the first part which I can easily implement by adding one line.

comment:6 Changed 20 months ago by wuthrich

We could have a minimal_twist function, too, if you wish, but I need minimal_quadratic_twist for modular symbols. If you can simplify the current function that is certainly a good thing to do.

I am not sure what changes you want to make, but if you do them, I should be able to look at them and review it quickly.

(from Besancon)

comment:7 Changed 20 months ago by cremona

• Branch set to u/cremona/ticket/14060
• Created changed from 02/05/13 03:56:34 to 02/05/13 03:56:34
• Modified changed from 01/11/14 09:17:31 to 01/11/14 09:17:31

comment:8 Changed 20 months ago by cremona

• Authors set to John Cremona
• Commit set to dc07e494c5230bef1dd7e1a0df351194a03acb36
• Description modified (diff)
• Status changed from new to needs_review

I have implemented the small improvement (do nothing if the conductor is square-free) with a new doctest, and also expanded on the documentation in the exceptional cases j=0, 1728 to remove any confusion, with an example!

I decided not to implement a new minimal_twist() function since

```EllipticCurve(j=E.j_invariant())
```

does that. But I could.

Last 10 new commits:

 ​520cfca Merge branch 'master' of git://github.com/sagemath/sage into build_system ​38d22b7 Merge branch 'master' of ssh://trac.sagemath.org:2222/sage into build_system ​139dcf5 Merge branch 'build_system' of git://github.com/sagemath/sage into build_system ​17e17dd Merge branch 'master' of ssh://trac.sagemath.org:2222/sage into build_system ​b06bd7c Merge branch 'master' of git://github.com/sagemath/sage ​858bb95 Merge branch 'master' of git://github.com/sagemath/sage ​0523e0d Merge branch 'master' of git://github.com/sagemath/sage ​88ffd55 Merge branch 'master' of git://github.com/sagemath/sage ​68d0a5d Merge branch 'master' of git://github.com/sagemath/sage ​057b3cd Merge branch 'master' of git://github.com/sagemath/sage

comment:9 Changed 20 months ago by cremona

There are lots of commits listed above, none of which is mine. The relevant one, which you can see by clicking on the branch name, is dc07e494c5230bef1dd7e1a0df351194a03acb36 .

comment:10 Changed 20 months ago by chapoton

the note field is only indented by 3 spaces, there should be 4

comment:11 Changed 19 months ago by wuthrich

• Branch changed from u/cremona/ticket/14060 to u/wuthrich/ticket/14060
• Commit changed from dc07e494c5230bef1dd7e1a0df351194a03acb36 to 634ab955474c1d1ed637171a96a85e3d26f3f5e0
• Reviewers set to Chris Wuthrich
• Status changed from needs_review to positive_review

I rebased it to the 6.1.beta6 in the hope that the extra merge commits above do not show anymore (even if they are harmless.

Since all test then pass for me, I give it a positive review.

New commits:

 ​634ab95 trac #14060: improvement to minimal_quadratic_twist

comment:12 follow-up: ↓ 13 Changed 19 months ago by wuthrich

By the way, I believe there are 4 spaces before the ..note. No ? At least the html output seems ok.

Last edited 19 months ago by wuthrich (previous) (diff)

comment:13 in reply to: ↑ 12 Changed 19 months ago by cremona

By the way, I believe there are 4 spaces before the ..note. No ? At least the html output seems ok.

I don't know, I thought that not warnings were produced when building the docs but have forgotten.

Thanks for the review!

comment:14 Changed 19 months ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

comment:15 Changed 19 months ago by vbraun

• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.