Opened 8 years ago

Closed 8 years ago

bug in simon_two_descent()

Reported by: Owned by: cremona cremona major sage-4.7 elliptic curves rlm, weigandt, gagansekhon sage-4.7.alpha5 John Cremona Chris Wuthrich N/A

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

Changed 8 years ago by cremona

Applies to 4.6.2.rc0

comment:1 Changed 8 years ago by cremona

• Status changed from new to needs_review

comment:2 Changed 8 years ago by 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)]


comment:3 Changed 8 years ago by cremona

• Cc rlm weigandt gagansekhon added

comment:4 follow-up: ↓ 5 Changed 8 years ago by 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).

comment:5 in reply to: ↑ 4 Changed 8 years ago by cremona

(* 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.

comment:6 follow-up: ↓ 7 Changed 8 years ago by wuthrich

• Reviewers set to Chris Wuthrich
• Status changed from needs_review to positive_review

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.

comment:7 in reply to: ↑ 6 Changed 8 years ago by jdemeyer

• Milestone set to sage-4.7
• Status changed from positive_review to needs_work

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.

comment:8 Changed 8 years ago by gagansekhon

• Status changed from needs_work to needs_review

Added the doctest for the wrong output.

comment:10 Changed 8 years ago by weigandt

• Status changed from needs_review to positive_review

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

spacing fix

comment:11 Changed 8 years ago by kini

• Description modified (diff)