Opened 4 years ago

Closed 3 years ago

#22719 closed defect (fixed)

integral points for elliptic curves broken

Reported by: wuthrich Owned by:
Priority: major Milestone: sage-8.2
Component: elliptic curves Keywords: integral points, ratpoints
Cc: cremona Merged in:
Authors: John Cremona Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: 5abadc7 (Commits) Commit: 5abadc71a145e3afce1a718da18452a3b611baa5
Dependencies: Stopgaps:

Description

sage: E = EllipticCurve("141d1")
sage: E.integral_points()

goes boom with --> 198 raise RuntimeError('Bad arguments to ratpoints')

Change History (10)

comment:1 Changed 4 years ago by wuthrich

I am not sure this has not been noted before in some form, but I could not find anything about it. It is the optimal curve of lowest conductor for which this appears.

comment:2 Changed 4 years ago by cremona

Thanks for spotting this. Since this was implemented I have checed many thousands of curves to compare with Magma, so this must be something new. In particular ratpoints has changed.

There's another ticket which has been around for ages in which I am fixing other integral points bugs. What joy.

comment:3 Changed 3 years ago by cremona

I just found this and took a look. It's easy to fix (though I don't know what has changed): in line 5770 of ell_rational_field.py ratpoints() does not like being given H=0 which it is in this case. I fixed this example by replacing the line above, defining H by

H = max(xmin.abs(), xmax.abs(), 1)

I am running some tests.

comment:4 Changed 3 years ago by cremona

  • Authors set to John Cremona
  • Branch set to u/cremona/22719
  • Commit set to 5abadc71a145e3afce1a718da18452a3b611baa5
  • Status changed from new to needs_review

New commits:

5abadc7#22719: fix bug in ratpoints call from integral_points

comment:5 Changed 3 years ago by cremona

I checked all curves of conductor <1000. Doing more checking now -- note that it's a lot quicker to run E.integral_points() on curves when you have the optional database_cremona_ellcurve installed since it does not have to find the Mordell-Weil group of each.

comment:6 Changed 3 years ago by roed

  • Reviewers set to David Roe
  • Status changed from needs_review to positive_review

Looks good.

comment:7 Changed 3 years ago by wuthrich

Oh, someone beat me to it :) I did random checks with thousands of curves and it always gives an answer now.

comment:8 Changed 3 years ago by cremona

Thanks both -- I meant to post again to say that I ran all curves up to conductor 105 with no problems.

comment:9 Changed 3 years ago by chapoton

  • Milestone changed from sage-8.0 to sage-8.2

comment:10 Changed 3 years ago by vbraun

  • Branch changed from u/cremona/22719 to 5abadc71a145e3afce1a718da18452a3b611baa5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.