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: |
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
Cc: | wuthrich added |
---|---|
Component: | PLEASE CHANGE → elliptic curves |
Keywords: | Cremona database added |
Type: | PLEASE CHANGE → defect |
comment:2 Changed 8 years ago by
Authors: | → John Cremona |
---|---|
Branch: | → u/cremona/17904 |
Commit: | → d8f530cd33b5ca3e102d8631992b0ad372a5d8c3 |
Status: | new → needs_review |
comment:3 follow-up: 6 Changed 8 years ago by
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
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
Status: | needs_review → positive_review |
---|
comment:6 follow-up: 7 Changed 8 years ago by
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 Changed 8 years ago by
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_ellcurveThere 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
Commit: | d8f530cd33b5ca3e102d8631992b0ad372a5d8c3 → 50a18a464d78ec580610cd34990e795a40725c6e |
---|---|
Status: | positive_review → needs_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
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
Branch: | u/cremona/17904 → u/wuthrich/17904 |
---|---|
Commit: | 50a18a464d78ec580610cd34990e795a40725c6e → 4de6008e45d19997207d59c686c180c717725d48 |
Status: | needs_review → positive_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:
4de6008 | trac 17904: small doc correction
|
comment:11 Changed 8 years ago by
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.
comment:12 Changed 8 years ago by
Branch: | u/wuthrich/17904 → 4de6008e45d19997207d59c686c180c717725d48 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
17904: fix ec database data extraction when num>1