Opened 5 years ago
Closed 5 years ago
#16456 closed defect (fixed)
Bug in descend_to method for elliptic curves
Reported by:  cremona  Owned by:  cremona 

Priority:  major  Milestone:  sage6.3 
Component:  elliptic curves  Keywords:  elliptic curve base change 
Cc:  Merged in:  
Authors:  John Cremona  Reviewers:  Peter Bruin 
Report Upstream:  N/A  Work issues:  
Branch:  e0a13b3 (Commits)  Commit:  e0a13b3931a2c0d0360a57a36a841cde96e00355 
Dependencies:  #16708  Stopgaps: 
Description (last modified by )
The function descend_to which was implemented in #9384 is incorrect. The twisting parameter d in L* is only defined modulo (L*)^2
and one has to determine whether there is a representative which lies in K*, which the implementation does not do, so some "positive" results are missed. Here is an example of this.
sage: K.<a> = NumberField(x^32) sage: E = EllipticCurve('11a1').quadratic_twist(2) sage: EK = E.change_ring(K) sage: EK2 = EK.change_weierstrass_model((a,a,a,a+1)) sage: EK2.descend_to(QQ) <None>
The heart of the problem being solved here is to determine whether (given that j(E) is in K of course) the twisting parameter in L*/(L*)^2
is in the image of K*/(K*)^2
or not, and the implementation ignores this. (For j=0 or 1728 the principle is the same with squares replaced by 6th or 4th powers respectively.)
I can fix this for number fields (one can restrict from K*/(K*)^2
to a finite K(S,2) for an easily determined set S of primes) or for finite fields, but it may not be possible to have this implemented for arbitrary fields, which will cause a problem with the patching.
A second and independent bug is reported by Warren Moore:
sage: k.<i> = QuadraticField(1) sage: E = EllipticCurve(k,[0,0,0,1,0]) sage: E.descend_to(QQ) == None True
This caused by a "naked Except" clause plus a call to f.preimage() for a map f which has no attribute/method "preimage".
Change History (26)
comment:1 Changed 5 years ago by
 Description modified (diff)
 Owner changed from (none) to cremona
comment:2 Changed 5 years ago by
 Description modified (diff)
comment:3 Changed 5 years ago by
 Description modified (diff)
comment:4 Changed 5 years ago by
 Branch set to u/cremona/bug_in_descend_to_method_for_elliptic_curves
comment:5 Changed 5 years ago by
 Commit set to 9b0a5d98c44957ef2aaed72b9f9af5389a4c2d41
 Status changed from new to needs_review
comment:6 Changed 5 years ago by
 Commit changed from 9b0a5d98c44957ef2aaed72b9f9af5389a4c2d41 to d5c4ba7cc88561e9715e2ac3f2e87e80decef459
Branch pushed to git repo; I updated commit sha1. New commits:
d5c4ba7  Merge branch 'develop' into t/16456/bug_in_descend_to_method_for_elliptic_curves

comment:7 Changed 5 years ago by
 Branch changed from u/cremona/bug_in_descend_to_method_for_elliptic_curves to u/cremona/16456
 Commit changed from d5c4ba7cc88561e9715e2ac3f2e87e80decef459 to 3db6c28c3ffb0c9d29eb5fd1e1fe5cd1acbd907f
comment:8 Changed 5 years ago by
 Reviewers set to Peter Bruin
I remember looking at the Selmer group code some time ago (and again just now) and getting the impression that the group K(S, m) returned by selmer_group(S, m)
is the one fitting in a natural exact sequence (warning: unicode experiment ahead)
1 ⟶ O_{K,S}^{×}/O_{K,S}^{×m} ⟶ K(S, m) ⟶ Cl_{K,S}[m] ⟶ 0.
(I.e., I think K(S, m) should be canonically isomorphic to the flat cohomology group H^{1}(O_{K}, μ_{m}). This can be replaced by étale cohomology if S contains all places dividing m, and in that case I also think that the above H^{1}, and K(S, m), should be isomorphic to H^{1}(Gal(K^{S}/K), μ_{m}) with K^{S} the maximal extension of K that is unramified outside S.)
Is the above exact sequence correct, and should it perhaps be mentioned in the documentation? And is there a comparable exact sequence for the "true" Selmer group consisting of the elements giving unramified extensions of K?
comment:9 followup: ↓ 10 Changed 5 years ago by
See http://trac.sagemath.org/ticket/16496 where you will find that exact sequence!
One thing which makes this sequence easier to deal with than one might expect is that the group in the middle is a direct product of the other two rather than some more complicated extension.
comment:10 in reply to: ↑ 9 Changed 5 years ago by
Replying to cremona:
See http://trac.sagemath.org/ticket/16496 where you will find that exact sequence!
Right! I am sure I have seen that ticket before...
One thing which makes this sequence easier to deal with than one might expect is that the group in the middle is a direct product of the other two rather than some more complicated extension.
That sounded surprising to me at first, but I tried to verify it and realised I had actually also done something very much like that before.
comment:11 Changed 5 years ago by
 Branch changed from u/cremona/16456 to u/pbruin/16456descend_to_bug
 Commit changed from 3db6c28c3ffb0c9d29eb5fd1e1fe5cd1acbd907f to 65d3404d1517a2c2005604c61582f163f0947c06
Here is a reviewer patch mostly consisting of documentation/formatting changes. If you agree with my changes, you can set it to positive review.
comment:12 Changed 5 years ago by
comment:13 Changed 5 years ago by
 Commit changed from 65d3404d1517a2c2005604c61582f163f0947c06 to bed05c7c4478314743ba2517888d44ff9e8064b6
Branch pushed to git repo; I updated commit sha1. New commits:
bed05c7  Trac 16456: corrected doctest changed by #16708

comment:14 Changed 5 years ago by
 Dependencies set to #16708
comment:15 Changed 5 years ago by
 Commit changed from bed05c7c4478314743ba2517888d44ff9e8064b6 to 65d3404d1517a2c2005604c61582f163f0947c06
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:16 followup: ↓ 17 Changed 5 years ago by
 Status changed from needs_review to needs_work
Actually both the original answer and my "corrected" one in the doctest changed by commit bed05c5
are wrong; if I'm not mistaken, the correct answer is
sage: K = QuadraticField(5) sage: list(K.selmer_group_iterator((), 4)) [1, 4, 1, 4]
comment:17 in reply to: ↑ 16 Changed 5 years ago by
Replying to pbruin:
Actually both the original answer and my "corrected" one in the doctest changed by commit
bed05c5
are wrong; if I'm not mistaken, the correct answer issage: K = QuadraticField(5) sage: list(K.selmer_group_iterator((), 4)) [1, 4, 1, 4]
That is certainly correct. I will look at this and #16708 tomorrow.
comment:18 followup: ↓ 21 Changed 5 years ago by
Between here and #16708 a doctest needed changing as I think Peter pointed out somewhere:
sage: K.<a> = QuadraticField(5) sage: list(K.selmer_group_iterator((), 4)) [1, 2, 4, 8, 1, 2, 4, 8]
should be
sage: K.<a> = QuadraticField(5) sage: list(K.selmer_group_iterator((), 4)) [1, 4, 16, 64, 1, 4, 16, 64]
which I am changing and will commit & push.
comment:19 Changed 5 years ago by
 Branch changed from u/pbruin/16456descend_to_bug to u/cremona/16456descend_to_bug
 Commit changed from 65d3404d1517a2c2005604c61582f163f0947c06 to e6ccee27c60c15aac6f103a0b5303450e36789c1
 Status changed from needs_work to needs_review
comment:20 Changed 5 years ago by
Peter, thanks for your tidying up of (and improving) the documentation. I just fixed that one doctest which I think you already noticed was incorrect because of the bug fixed now at #16708. That means that the branch name has gone back to mine (u/cremona/...), and I have set it back to "needs review". Since I like your reviewer's changes, if you agree wit hthis last one then we can set it to "positive review".
comment:21 in reply to: ↑ 18 Changed 5 years ago by
 Status changed from needs_review to needs_work
Replying to cremona:
Between here and #16708 a doctest needed changing as I think Peter pointed out somewhere:
sage: K.<a> = QuadraticField(5) sage: list(K.selmer_group_iterator((), 4)) [1, 2, 4, 8, 1, 2, 4, 8]should be
sage: K.<a> = QuadraticField(5) sage: list(K.selmer_group_iterator((), 4)) [1, 4, 16, 64, 1, 4, 16, 64]which I am changing and will commit & push.
That second answer was actually the wrong "correction" I pushed and then retracted. It should be [1, 4, 1, 4]
(as in comment:17); in fact 16 is a fourth power, hence represents the trivial element of the Selmer group. The Selmer group is isomorphic to C_{2} x C_{2} (since both the units modulo 4th powers and the 4torsion in the class group are cyclic of order 2.)
I think the problem is the line
f = lambda o: m if o is Infinity else o.gcd(m)
where you will somehow need to take the orders of elements of the class group into account.
comment:22 Changed 5 years ago by
Sorry about that. I'll work out how to correct it....
comment:23 Changed 5 years ago by
 Commit changed from e6ccee27c60c15aac6f103a0b5303450e36789c1 to e0a13b3931a2c0d0360a57a36a841cde96e00355
Branch pushed to git repo; I updated commit sha1. New commits:
e0a13b3  #16456: fix bug in orders of Selmer group elements

comment:24 Changed 5 years ago by
 Status changed from needs_work to needs_review
...done. I added an optional parameter "orders" to the selmer_group method, default False gives the old behavious, if True also outputs a list of the orders of the generators, which are all equal to m when m is prime, but not necesarily otherwise. Added doctests for this. Now the iterator uses this instead of the old formula, which was wrong. I thought that this was simpler than trying to recover the orders from the list of generators, since it is information we have to hand when constructing the list of generators.
For consistency I did the same for the special versions over QQ, though the bug did not apply here. At the same time I noticed that over QQ when 1 is a generator it was put last while over number fields the units are always first, so I changed that (and the associated doctests) too.
Note that this change will be superceded after #16496 when there is a proper class for these Selmer groups. And also that the correctness of the current selmer_group function relies on the direct product fact alluded to in earlier comments!
comment:25 Changed 5 years ago by
 Status changed from needs_review to positive_review
OK, everything looks good to me now!
comment:26 Changed 5 years ago by
 Branch changed from u/cremona/16456descend_to_bug to e0a13b3931a2c0d0360a57a36a841cde96e00355
 Resolution set to fixed
 Status changed from positive_review to closed
Ready for review. Note that there are two separate things here (which perhaps should have been in separate tickets):
Up till the last commit the changes are in in number fields (including QQ), not touching elliptic curves, implementing the function descend_mod_power for number field elements and enhancing the selmer_group with selmer_group_iterator for ease of use. (Also correcting the documentation for selmer_group).
The last commit does what the ticket wanted, namely to reimplement the descend_to function for elliptic curves. Apart from trivial cases this will only work over number fields. It would work over any other field where descend_mod_power could be implemented such as finite fields, but note that in characteristics 2 and 3 the function would need to have a completely different implementation.
New commits:
Enhanced number field selmer group capabilities by adding an iterator
#16456: fix bugs in elliptic curve descend_to function