Opened 7 years ago

Closed 7 years ago

#18031 closed defect (fixed)

Bug in saturation for elliptic curves over Q

Reported by: cremona Owned by:
Priority: blocker Milestone: sage-6.6
Component: elliptic curves Keywords: saturation
Cc: Merged in:
Authors: John Cremona Reviewers: François Bissey
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: acefe94 (Commits, GitHub, GitLab) Commit: acefe94db857b8ae9de051203a5a61ff74df7d0c
Dependencies: Stopgaps:

Status badges

Description (last modified by cremona)

A bug in eclib's saturation function is revealed here: 

sage: E = EllipticCurve([0,-1,1,-266,968])
sage: Q1 = E([-1995,3674,125])
sage: Q2 = E([157,1950,1])
sage: E.saturation([Q1,Q2])                                                                                     
([(-399/25 : 3674/125 : 1), (157 : 1950 : 1)], 1, 7.21429780216482)
sage: P1, P2 = E.gens()                                                                                         
sage: E.regulator()
0.801588644684980
sage: E.regulator_of_points([Q1,Q2])
7.21429780216482

The points Q1,Q2 generate a subgroup of index 3 but the computed bound on the index is between 2 and 3. This will be fixed upstream.

Note that for this curve E.gens() gives correct generators but E.simon_two_descent() gives the above two points.

New upstream source at ​http://boxen.math.washington.edu/home/cremona/eclib-20150323.tar.bz2

Change History (10)

comment:1 Changed 7 years ago by cremona

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.

Bug in eclib fixed. After testing, will update the upstream sources and the package information.

comment:2 Changed 7 years ago by cremona

  • Description modified (diff)

comment:3 Changed 7 years ago by cremona

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

With upstream eclib-20150323: 

sage: E = EllipticCurve([0,-1,1,-266,968])
sage: Q1 = E([-1995,3674,125])
sage: Q2 = E([157,1950,1])
sage: E.saturation([Q1,Q2])
([(1 : -27 : 1), (157 : 1950 : 1)], 3, 0.801588644684981)

which will be put in as a doctest.

comment:4 Changed 7 years ago by cremona

  • Branch set to u/cremona/18031

comment:5 Changed 7 years ago by cremona

  • Branch changed from u/cremona/18031 to u/cremona/eclib-18031
  • Commit set to acefe94db857b8ae9de051203a5a61ff74df7d0c

New commits:

​acefe94update eclib to bugfix release 20150323 and add doctest

comment:6 Changed 7 years ago by cremona

  • Status changed from new to needs_review

comment:7 Changed 7 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

lgtm

comment:8 Changed 7 years ago by cremona

Thanks!

Release manager -- compared to 20150228 which was already merged, this only changes one source file, there are no API changes so this is a minor bug-fix release. Sorry it comes so soon after the previous one, but that's when I discovered the bug.

comment:9 Changed 7 years ago by fbissey

  • Priority changed from major to blocker

I am making this a blocker, that'll get Volker's attention.

comment:10 Changed 7 years ago by vbraun

  • Branch changed from u/cremona/eclib-18031 to acefe94db857b8ae9de051203a5a61ff74df7d0c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.