CremonaDatabase omits data for curves not first in their class

[JohnCremona]

Chris Wuthrich reported that

sage: E = EllipticCurve('100467a2')
sage: E.database_attributes()
{'conductor': 100467,
 'cremona_label': '100467a2',
 'rank': 1,
 'torsion_order': 1}

although the database does contain the generators:

sage: CremonaDatabase().allgens(100467)['a2']
[[7465870518064287, 219103670535029299, 7758864174573]]

It seems that (in two palces) the code to extract the data from the database wrongly assumes that some data only exists for the first curve in each isogeny class, which is not true.

A simple fix to sage/databases/cremona.py is on its way.

CC: @categorie

Component: elliptic curves

Keywords: Cremona database

Author: John Cremona

Branch/Commit: 4de6008

Reviewer: Chris Wuthrich

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

[JohnCremona]

Changed keywords from none to Cremona database

[JohnCremona]

Branch: u/cremona/17904

[JohnCremona]

New commits:

d8f530c

17904: fix ec database data extraction when num>1

[JohnCremona]

Commit: d8f530c

[JohnCremona]

Author: John Cremona

[JohnCremona]

comment:3

I did not add a doctest since it can only be tested with the optional database_cremona_ellcurve installed.

[chriswuthrich]

comment:4

I agree with the fix and I checked that it solves the problem. I am running the obligatory tests now.

Thanks for fixing that so fast. Very helpful.

[chriswuthrich]

Reviewer: Chris Wuthrich

[jdemeyer]

comment:6

Replying to @JohnCremona:

I did not add a doctest since it can only be tested with the optional database_cremona_ellcurve installed.

You should add a doctest and annotate it with

# optional - database_cremona_ellcurve

There are many such tests already in src/sage/databases/cremona.py.

(Note that I am not setting the ticket to needs_work for this)

[JohnCremona]

comment:7

Replying to @jdemeyer:

Replying to @JohnCremona:

I did not add a doctest since it can only be tested with the optional database_cremona_ellcurve installed.

You should add a doctest and annotate it with

# optional - database_cremona_ellcurve

There are many such tests already in src/sage/databases/cremona.py.

OK, I will do that!

(Note that I am not setting the ticket to needs_work for this)

[sagetrac-git]

Changed commit from d8f530c to 50a18a4

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

50a18a4

#17904 added doctests

[JohnCremona]

comment:9

OK, so I added the doctest and the act of pushing the new commit automatically caused the needs_review flag to be set. Sorry, Chris!

[chriswuthrich]

Changed commit from 50a18a4 to 4de6008

[chriswuthrich]

Changed branch from u/cremona/17904 to u/wuthrich/17904

[chriswuthrich]

comment:10

Ok. I fixed a tiny sphinx error on that page, too.

I did test cremona.py, but I have not run the complete test again after the last changes which only affect the docstring in that file. If anyone finds that inacceptable, then I will run them.

---

New commits:

4de6008

trac 17904: small doc correction

[JohnCremona]

comment:11

Thanks -- I think we can leave the rest to various bots.

For what it's worth I used the cremona_curves() iterator to grab every single curve and check that len(E.gens())==E.rank(), which it did fast enough to be confident that all the data was really coming from the database.

[vbraun]

Changed branch from u/wuthrich/17904 to 4de6008