Opened 11 years ago
Closed 10 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 )
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)
Change History (16)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
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
- Cc rlm weigandt gagansekhon added
comment:4 follow-up: ↓ 5 Changed 11 years ago by
(* 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
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: ↓ 7 Changed 11 years ago by
- 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
- 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
- Status changed from needs_work to needs_review
Added the doctest for the wrong output.
Changed 11 years ago by
comment:9 Changed 11 years ago by
Please use trac_10872.patch, it is the latest version of the patch.
comment:10 Changed 11 years ago by
- Status changed from needs_review to positive_review
Tests pass, documentation of the offending example has been added.
comment:11 Changed 11 years ago by
- Description modified (diff)
Added a trivial spacing fix. Added a comment for the patchbot to read.
comment:12 Changed 11 years ago by
- Description modified (diff)
Whoops, comment for the patchbot fixed
comment:13 Changed 10 years ago by
- Merged in set to sage-4.7.alpha5
- Resolution set to fixed
- Status changed from positive_review to closed
Applies to 4.6.2.rc0