Opened 3 years ago
Closed 2 years ago
#14060 closed defect (fixed)
Improve minimal_quadratic_twist()
Reported by: | cremona | Owned by: | cremona |
---|---|---|---|
Priority: | major | Milestone: | sage-6.2 |
Component: | elliptic curves | Keywords: | pari precision |
Cc: | wuthrich | Merged in: | |
Authors: | John Cremona | Reviewers: | Chris Wuthrich |
Report Upstream: | N/A | Work issues: | |
Branch: | u/wuthrich/ticket/14060 (Commits) | Commit: | 634ab955474c1d1ed637171a96a85e3d26f3f5e0 |
Dependencies: | Stopgaps: |
Description (last modified by cremona)
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.
Change History (15)
comment:1 Changed 2 years ago by jdemeyer
- Milestone changed from sage-5.11 to sage-5.12
comment:2 Changed 2 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()
comment:3 Changed 2 years ago by cremona
- Cc wuthrich added
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 2 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 2 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 2 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 2 years 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 2 years ago by 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 2 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 2 years ago by chapoton
the note field is only indented by 3 spaces, there should be 4
comment:11 Changed 2 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 2 years ago by wuthrich
By the way, I believe there are 4 spaces before the ..note. No ? At least the html output seems ok.
comment:13 in reply to: ↑ 12 Changed 2 years ago by cremona
Replying to wuthrich:
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 2 years ago by vbraun_spam
- Milestone changed from sage-6.1 to sage-6.2
comment:15 Changed 2 years ago by vbraun
- Resolution set to fixed
- Status changed from positive_review to closed
Precision issue is #13163.