Opened 5 years ago

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

• Milestone changed from sage-5.11 to sage-5.12

### comment:2 Changed 4 years 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 4 years 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 4 years 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 4 years 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 4 years 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 4 years ago by cremona

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

### comment:8 Changed 4 years 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 4 years 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 4 years ago by chapoton

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

### comment:11 Changed 4 years 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 4 years 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 4 years ago by wuthrich (previous) (diff)

### comment:13 in reply to: ↑ 12 Changed 4 years 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 4 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:15 Changed 4 years ago by vbraun

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