Opened 8 years ago

Closed 8 years ago

#10832 closed defect (fixed)

bug in simon_two_descent()

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-4.7
Component: elliptic curves Keywords:
Cc: rlm, weigandt, gagansekhon Merged in: sage-4.7.alpha5
Authors: John Cremona Reviewers: Chris Wuthrich
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kini)

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

Attachments (3)

trac_10832-descent.patch (1023 bytes) - added by cremona 8 years ago.
Applies to 4.6.2.rc0
trac_10872.patch (1.6 KB) - added by gagansekhon 8 years ago.
trac_10872-descent.v3.patch (1.6 KB) - added by kini 8 years ago.
spacing fix

Download all attachments as: .zip

Change History (16)

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

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.

comment:6 follow-up: 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

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.

comment:8 Changed 8 years ago by gagansekhon

  • Status changed from needs_work to needs_review

Added the doctest for the wrong output.

Changed 8 years ago by gagansekhon

comment:9 Changed 8 years ago by gagansekhon

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

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.

Changed 8 years ago by kini

spacing fix

comment:11 Changed 8 years ago by kini

  • Description modified (diff)

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

comment:12 Changed 8 years ago by kini

  • Description modified (diff)

Whoops, comment for the patchbot fixed

comment:13 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.