Opened 8 years ago

Closed 8 years ago

#17904 closed defect (fixed)

CremonaDatabase omits data for curves not first in their class

Reported by: John Cremona Owned by:
Priority: major Milestone: sage-6.6
Component: elliptic curves Keywords: Cremona database
Cc: wuthrich Merged in:
Authors: John Cremona Reviewers: Chris Wuthrich
Report Upstream: N/A Work issues:
Branch: 4de6008 (Commits, GitHub, GitLab) Commit: 4de6008e45d19997207d59c686c180c717725d48
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (12)

comment:1 Changed 8 years ago by John Cremona

Cc: wuthrich added
Component: PLEASE CHANGEelliptic curves
Keywords: Cremona database added
Type: PLEASE CHANGEdefect

comment:2 Changed 8 years ago by John Cremona

Authors: John Cremona
Branch: u/cremona/17904
Commit: d8f530cd33b5ca3e102d8631992b0ad372a5d8c3
Status: newneeds_review

New commits:

d8f530c17904: fix ec database data extraction when num>1

comment:3 Changed 8 years ago by John Cremona

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

comment:4 Changed 8 years ago by wuthrich

Reviewers: Chris Wuthrich

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.

comment:5 Changed 8 years ago by wuthrich

Status: needs_reviewpositive_review

comment:6 in reply to:  3 ; Changed 8 years ago by Jeroen Demeyer

Replying to cremona:

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)

comment:7 in reply to:  6 Changed 8 years ago by John Cremona

Replying to jdemeyer:

Replying to cremona:

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)

comment:8 Changed 8 years ago by git

Commit: d8f530cd33b5ca3e102d8631992b0ad372a5d8c350a18a464d78ec580610cd34990e795a40725c6e
Status: positive_reviewneeds_review

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

50a18a4#17904 added doctests

comment:9 Changed 8 years ago by John Cremona

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!

comment:10 Changed 8 years ago by wuthrich

Branch: u/cremona/17904u/wuthrich/17904
Commit: 50a18a464d78ec580610cd34990e795a40725c6e4de6008e45d19997207d59c686c180c717725d48
Status: needs_reviewpositive_review

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:

4de6008trac 17904: small doc correction

comment:11 Changed 8 years ago by John Cremona

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.

Last edited 8 years ago by John Cremona (previous) (diff)

comment:12 Changed 8 years ago by Volker Braun

Branch: u/wuthrich/179044de6008e45d19997207d59c686c180c717725d48
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.