Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

integral points for elliptic curves broken #22719

Closed
chriswuthrich opened this issue Mar 30, 2017 · 13 comments
Closed

integral points for elliptic curves broken #22719

chriswuthrich opened this issue Mar 30, 2017 · 13 comments

Comments

@chriswuthrich
Copy link
Contributor

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

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

CC: @JohnCremona

Component: elliptic curves

Keywords: integral points, ratpoints

Author: John Cremona

Branch/Commit: 5abadc7

Reviewer: David Roe

Issue created by migration from https://trac.sagemath.org/ticket/22719

@chriswuthrich
Copy link
Contributor Author

comment:1

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.

@JohnCremona
Copy link
Member

comment:2

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.

@JohnCremona
Copy link
Member

comment:3

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.

@JohnCremona
Copy link
Member

Author: John Cremona

@JohnCremona
Copy link
Member

Commit: 5abadc7

@JohnCremona
Copy link
Member

Branch: u/cremona/22719

@JohnCremona
Copy link
Member

New commits:

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

@JohnCremona
Copy link
Member

comment:5

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.

@roed314
Copy link
Contributor

roed314 commented Nov 8, 2017

Reviewer: David Roe

@roed314
Copy link
Contributor

roed314 commented Nov 8, 2017

comment:6

Looks good.

@chriswuthrich
Copy link
Contributor Author

comment:7

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

@JohnCremona
Copy link
Member

comment:8

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

@fchapoton fchapoton modified the milestones: sage-8.0, sage-8.2 Nov 30, 2017
@vbraun
Copy link
Member

vbraun commented Dec 11, 2017

Changed branch from u/cremona/22719 to 5abadc7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants