Opened 5 years ago
Closed 4 years ago
#14060 closed defect (fixed)
Improve minimal_quadratic_twist()
Reported by:  cremona  Owned by:  cremona 

Priority:  major  Milestone:  sage6.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 )
We need a better implementation of minimal_quadratic_twist(), for example if the conductor is squarefree 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 4 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:2 Changed 4 years ago by
 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 4 years ago by
 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 jinvariants 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 CCing 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
In sha_tate and in general for padic Lfunctions 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
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
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
 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
 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 squarefree) 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
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
the note
field is only indented by 3 spaces, there should be 4
comment:11 Changed 4 years ago by
 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 followup: ↓ 13 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:15 Changed 4 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
Precision issue is #13163.