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: sage-6.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 cremona)

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^3-2)
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 cremona

  • Description modified (diff)
  • Owner changed from (none) to cremona

comment:2 Changed 5 years ago by cremona

  • Description modified (diff)

comment:3 Changed 5 years ago by cremona

  • Description modified (diff)

comment:4 Changed 5 years ago by cremona

  • Branch set to u/cremona/bug_in_descend_to_method_for_elliptic_curves

comment:5 Changed 5 years ago by cremona

  • Authors set to John Cremona
  • Commit set to 9b0a5d98c44957ef2aaed72b9f9af5389a4c2d41
  • Status changed from new to needs_review

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 re-implement 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:

04c1c45Enhanced number field selmer group capabilities by adding an iterator
9b0a5d9#16456: fix bugs in elliptic curve descend_to function

comment:6 Changed 5 years ago by git

  • Commit changed from 9b0a5d98c44957ef2aaed72b9f9af5389a4c2d41 to d5c4ba7cc88561e9715e2ac3f2e87e80decef459

Branch pushed to git repo; I updated commit sha1. New commits:

d5c4ba7Merge branch 'develop' into t/16456/bug_in_descend_to_method_for_elliptic_curves

comment:7 Changed 5 years ago by cremona

  • Branch changed from u/cremona/bug_in_descend_to_method_for_elliptic_curves to u/cremona/16456
  • Commit changed from d5c4ba7cc88561e9715e2ac3f2e87e80decef459 to 3db6c28c3ffb0c9d29eb5fd1e1fe5cd1acbd907f

New commits:

12c1b73minor addition to isogeny_small_degree for sporadic primes over number fields
1c3ddb9Merge branch 'develop' into isogs
3db6c28Merge branch 'develop' into isogs

comment:8 Changed 5 years ago by pbruin

  • 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 ⟶ OK,S×/OK,S×mK(S, m) ⟶ ClK,S[m] ⟶ 0.

(I.e., I think K(S, m) should be canonically isomorphic to the flat cohomology group H1(OK, μ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 H1, and K(S, m), should be isomorphic to H1(Gal(KS/K), μm) with KS 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 follow-up: Changed 5 years ago by cremona

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 pbruin

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 pbruin

  • Branch changed from u/cremona/16456 to u/pbruin/16456-descend_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 pbruin

While trying to find a clever cohomological argument for the splitting of the exact sequence from comment:8, I did not succeed but did find a bug in selmer_group(); see #16708.

comment:13 Changed 5 years ago by git

  • Commit changed from 65d3404d1517a2c2005604c61582f163f0947c06 to bed05c7c4478314743ba2517888d44ff9e8064b6

Branch pushed to git repo; I updated commit sha1. New commits:

bed05c7Trac 16456: corrected doctest changed by #16708

comment:14 Changed 5 years ago by pbruin

  • Dependencies set to #16708

comment:15 Changed 5 years ago by git

  • Commit changed from bed05c7c4478314743ba2517888d44ff9e8064b6 to 65d3404d1517a2c2005604c61582f163f0947c06

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

comment:16 follow-up: Changed 5 years ago by pbruin

  • 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 cremona

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 is

sage: 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 follow-up: Changed 5 years ago by 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.

comment:19 Changed 5 years ago by cremona

  • Branch changed from u/pbruin/16456-descend_to_bug to u/cremona/16456-descend_to_bug
  • Commit changed from 65d3404d1517a2c2005604c61582f163f0947c06 to e6ccee27c60c15aac6f103a0b5303450e36789c1
  • Status changed from needs_work to needs_review

New commits:

8474e25Trac 16708: fix computation of Selmer groups of number fields
c584016Merge remote-tracking branch 'trac/u/pbruin/16456-descend_to_bug' into 16708
e6ccee2#16456: fix one doctest after #16708

comment:20 Changed 5 years ago by cremona

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 pbruin

  • 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 C2 x C2 (since both the units modulo 4-th powers and the 4-torsion 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 cremona

Sorry about that. I'll work out how to correct it....

comment:23 Changed 5 years ago by git

  • 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 cremona

  • 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 pbruin

  • Status changed from needs_review to positive_review

OK, everything looks good to me now!

comment:26 Changed 5 years ago by vbraun

  • Branch changed from u/cremona/16456-descend_to_bug to e0a13b3931a2c0d0360a57a36a841cde96e00355
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.