bug in simon_two_descent()

[cremona]

This is similar to, but different to, #10745.

sage: E = EllipticCurve([1,0,0,-6664,86543])
sage: E.simon_two_descent()
(2, 3, [(173 : 1943 : 1), (-73 : -394 : 1), (323/4 : 1891/8 : 1)])
sage: E.rank()
3
sage: E.gens()
[(-73 : -394 : 1), (323/4 : 1891/8 : 1), (173 : 1943 : 1)]

This is _wrong_. The rank is 2; the first point in the output has order 4. The mistake is that in discarding torsion points from the generators of E/2E, only points of exact order 2 are omitted!

Apply trac_10872-descent.v3.patch

[cremona]

Applies to 4.6.2.rc0

[cremona]

After the patch:

sage: E = EllipticCurve([1,0,0,-6664,86543])
sage: E.simon_two_descent()
(2, 3, [(173 : 1943 : 1), (-73 : -394 : 1), (323/4 : 1891/8 : 1)])
sage: E.rank()
2
sage: E.gens()
[(-73 : -394 : 1), (323/4 : 1891/8 : 1)]

[shin]

(* first comment ever)

Works fine on snow leopard 10.6.6.

But I'm not sure I understand. Sure, the "rank" should be returned as 2, but if I ask for the generators - shouldn't we get the generators of the torsion as well? I'm simply thinking of consistency with other groups such as unit groups. The rank of the unit group of the cyclotomic field QQ(\zeta_5) is 1, but there are two generators (and sage does return 2).

[cremona]

Replying to shin:

(* first comment ever)

Works fine on snow leopard 10.6.6.

But I'm not sure I understand. Sure, the "rank" should be returned as 2, but if I ask for the generators - shouldn't we get the generators of the torsion as well? I'm simply thinking of consistency with other groups such as unit groups. The rank of the unit group of the cyclotomic field QQ(\zeta_5) is 1, but there are two generators (and sage does return 2).

Welcome!

You are absolutely right from the mathematical point of view, the generators should really include 0, 1 or 2 torsion generators. But there is a tradition with elliptic curves of using "generators" to mean "generators modulo torsion", and I think that to change the behaviour to what you suggest would break a lot of code, hence a Bad Thing.

On the other hand it would be very reasonable to expect the docstring to be more precise, and in fact it is: for curves over Q the docstring for gens clearly states that it returns generaotors modulo torsion; while for curves over other number fields it just says

Returns some generators of this elliptic curve. Check ``rank()`` or
       ``rank_bounds()`` to verify the number of generators.

so the user is not being misled.

[wuthrich]

Looks good to me. All tests passed.

There is no new doctest in it, but I checked carefully on examples that it now computes the rank correctly.

[jdemeyer]

Replying to wuthrich:

Looks good to me. All tests passed.

There is no new doctest in it, but I checked carefully on examples that it now computes the rank correctly.

Well, I think at the very least the "wrong" output from the ticket description should be doctested.

[gagansekhon]

Added the doctest for the wrong output.

[gagansekhon]

Please use trac_10872.patch, it is the latest version of the patch.

[weigandt]

Tests pass, documentation of the offending example has been added.

[kini]

spacing fix

[kini]

Added a trivial spacing fix. Added a comment for the patchbot to read.

[kini]

Whoops, comment for the patchbot fixed