Opened 11 years ago

Last modified 11 years ago

#10832 closed defect

bug in simon_two_descent() — at Version 11

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

Status badges

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.patch

Change History (14)

Changed 11 years ago by cremona

Applies to 4.6.2.rc0

comment:1 Changed 11 years ago by cremona

  • Status changed from new to needs_review

comment:2 Changed 11 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 11 years ago by cremona

  • Cc rlm weigandt gagansekhon added

comment:4 follow-up: Changed 11 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 11 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 11 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 11 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 11 years ago by gagansekhon

  • Status changed from needs_work to needs_review

Added the doctest for the wrong output.

Changed 11 years ago by gagansekhon

comment:9 Changed 11 years ago by gagansekhon

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

comment:10 Changed 11 years ago by weigandt

  • Status changed from needs_review to positive_review

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

Changed 11 years ago by kini

spacing fix

comment:11 Changed 11 years ago by kini

  • Description modified (diff)

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

Note: See TracTickets for help on using tickets.