Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#11587 closed enhancement (fixed)

update Cremona's tables for Sage

Reported by: was Owned by: cremona
Priority: major Milestone: sage-4.8
Component: elliptic curves Keywords: elliptic curves ellcurve database
Cc: davidloeffler Merged in: sage-4.8.alpha0
Authors: R. Andrew Ohana Reviewers: John Cremona, Tom Boothby
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11642 Stopgaps:

Description (last modified by jdemeyer)

John Cremona tells me that: "All data for elliptic curves of conductors from 130k to 200k is at http://www.warwick.ac.uk/staff/J.E.Cremona/ftp/data/ and also as a tar file at http://www.warwick.ac.uk/staff/J.E.Cremona/ftp/ecdata-2011-10-26.tgz -- not to mention at http://code.google.com/p/ecdata/ !"

The goal of this ticket is to:

(1) Create a new drop-in-replacement sqlite database that has the data up to level 10000, which will be included standard with Sage.

(2) Create a new sqlite database that has the data up to level 200000, which will be an optional spkg.

This is complicated because the current packages in Sage use ZODB. Also, Cremona's database format has changed somewhat.

There is now a patch that depends upon #11642 (which depends upon #11640).

You can find updated elliptic_curve and database_cremona_ellcurve spkgs at http://wstein.org/home/ohanar/cremona-database/.


New spkg: http://sage.math.washington.edu/home/leif/Sage/spkgs/elliptic_curves-0.3.spkg

Apply trac_11587.patch to the Sage library.

Apply trac_11587-docfixes.patch to the Sage library.

Apply trac_11587-rmhardcode.proper.patch to the Sage library.

Apply trac_11587-fix_speed_regression.patch to the Sage library.

Apply trac_11587-depfix.patch to Sage root.

Optional spkg: http://wstein.org/home/ohanar/cremona-database/database_cremona_ellcurve-20111029.spkg

Attachments (6)

trac_11587.patch (87.3 KB) - added by ohanar 9 years ago.
fixes the broken doctest for cremona_mini
trac_11587-depfix.patch (889 bytes) - added by ohanar 9 years ago.
Adds the new dependencies of the elliptic_curves spkg
trac_11587-docfixes.patch (4.4 KB) - added by ohanar 9 years ago.
Apply on top to sage library. Fixes issues with documentation.
trac_11587-rmhardcode.patch (3.2 KB) - added by ohanar 9 years ago.
removes the hardcoded version number from sage library
trac_11587-rmhardcode.proper.patch (3.2 KB) - added by leif 9 years ago.
"Proper" Mercurial changeset replacement patch. (Adds just the "User" field.)
trac_11587-fix_speed_regression.patch (745 bytes) - added by davidloeffler 9 years ago.
apply over previous patches

Download all attachments as: .zip

Change History (72)

comment:1 Changed 10 years ago by ohanar

  • Description modified (diff)

Updated description to reflect the current status of the tables.

comment:2 Changed 10 years ago by cremona

Note: only the allgens file format has changed.

comment:3 Changed 9 years ago by ohanar

  • Description modified (diff)

comment:4 Changed 9 years ago by ohanar

  • Status changed from new to needs_review

comment:5 follow-up: Changed 9 years ago by cremona

  • Reviewers set to John Cremona
  • Status changed from needs_review to needs_info

I have a few questions. Also since I do not know SQLITE and do not have the time to learn it just for this, the code will need to be reviewed by someone else.

There are two spkgs at the link, and one patch on this ticket. Please can you explain how they are related? Is one of the spkgs intended to be part of the standard Sage distribution so that every copy of Sage has the mini-database, as is the case now? And is the other one a replacement for the optional large database?

Secondly, you warn that the database takes some time to create. Is this once-only time which you had to spend in creating the database which is part of the spkg, or is it time which must be spent by anyone installing the large optional spkg?

To test this, I assume that the following steps are necessary; please correct me if I am wrong.

  1. apply patches at #11642
  2. apply the patch here (trac_11587.patch)
  3. sage -b
  4. sage -i http://wstein.org/home/ohanar/cremona-database/elliptic_curves-0.2.spkg
  5. sage -b, test everything works
  6. sage -i http://wstein.org/home/ohanar/cremona-database/database_cremona_ellcurve-20110801.spkg
  7. sage -b, test everything still works

Lastly: I expect that at regular intervals over the next few months I will be releasing updates to the database (for example, the range 170k=180k is about 70% done already). Please leave rather clear instructions for what to do for future updates; in particular, will it be neceaary to rebuild everything, or just to read in the new files (e.g. *.170000-179999 next time)?

comment:6 in reply to: ↑ 5 Changed 9 years ago by ohanar

  • Description modified (diff)
  • Status changed from needs_info to needs_review

Replying to cremona:

There are two spkgs at the link, and one patch on this ticket. Please can you explain how they are related?

The patch updates sage.databases.cremona to wrap the new SQLite databases. It provides about the same functionality as the old ZODB (there are some slight modifications to actually reflect the documentation, which wasn't respected before -- specifically the mini-database only has data from allcurves).

Is one of the spkgs intended to be part of the standard Sage distribution so that every copy of Sage has the mini-database, as is the case now? And is the other one a replacement for the optional large database?

Correct. The elliptic_curve spkg is included every copy of sage, and includes both the mini-database, as well as a very small database of curves William finds interesting. The database_cremona_ellcurve spkg includes (most of) the data from allcurves, allgens, allbsd, and degphi for every curve that you have listed.

Secondly, you warn that the database takes some time to create. Is this once-only time which you had to spend in creating the database which is part of the spkg, or is it time which must be spent by anyone installing the large optional spkg?

Currently the spkgs simply install pre-built SQL databases, so currently this is a once-only thing. That said, on my home system it only takes ~4mins to create the database from scratch.

To test this, I assume that the following steps are necessary; please correct me if I am wrong.

  1. apply patches at #11642
  2. apply the patch here (trac_11587.patch)
  3. sage -b
  4. sage -i http://wstein.org/home/ohanar/cremona-database/elliptic_curves-0.2.spkg
  5. sage -b, test everything works
  6. sage -i http://wstein.org/home/ohanar/cremona-database/database_cremona_ellcurve-20110801.spkg
  7. sage -b, test everything still works

Yes, that looks correct. The only thing to be aware of is that #11642 depends on #11640 (which is just a small patch), and that there are optional doctests you should run in step 7.

Lastly: I expect that at regular intervals over the next few months I will be releasing updates to the database (for example, the range 170k=180k is about 70% done already). Please leave rather clear instructions for what to do for future updates; in particular, will it be neceaary to rebuild everything, or just to read in the new files (e.g. *.170000-179999 next time)?

Currently there is simple script to rebuild everything, but (in theory) it is also possible to simply add the new files. The former:

rm SAGE_ROOT/data/cremona/cremona.db
SAGE_ROOT/sage
sage: sage.databases.cremona.build('cremona','/path/to/ecdata.tgz')

the later should be (although I haven't tested it):

make a directory that only contains the new files
SAGE_ROOT/sage
sage: cData = sage.databases.cremona.LargeCremonaDatabase('cremona',False)
sage: cData._init_from_ftpdata('/path/to/said/directory')

That said, I've marked myself as the maintainer of the database_cremona_ellcurve spkg, so I'll try to keep it up to date (of course it would help if you let me know when you update your tables of course (: ).

comment:7 Changed 9 years ago by ohanar

  • Description modified (diff)

I've updated the database_cremona_ellcurve spkg to include the latest tables, which can still be found at http://wstein.org/home/ohanar/cremona-database (the old one is still there there as well).

I'm also attaching an updated patch that mainly updates the documentation to reflect the current state of the tables (upper bound of 180,000 vs 130,000), as well as the spkg version. It also limits the outputs of a few queries where only the first entry is needed.

Changed 9 years ago by ohanar

fixes the broken doctest for cremona_mini

comment:8 Changed 9 years ago by boothby

  • Authors set to Andrew Ohana
  • Reviewers changed from John Cremona to John Cremona, Tom Boothby
  • Status changed from needs_review to positive_review

looks good, works good

comment:9 follow-up: Changed 9 years ago by leif

  • Authors changed from Andrew Ohana to R. Andrew Ohana
  • Dependencies set to #11642
  • Description modified (diff)
  • Keywords elliptic curves ellcurve database added

That's +1.4 MB, or +78% for the standard spkg.

comment:10 in reply to: ↑ 9 Changed 9 years ago by ohanar

Replying to leif:

That's +1.4 MB, or +78% for the standard spkg.

I can rewrite the install script to simply create the database from Cremona's text files. On my system it takes ~6sec to build it from scratch, and would probably decrease the size of the spkg to a bit over 1MB.

Changed 9 years ago by ohanar

Adds the new dependencies of the elliptic_curves spkg

comment:11 follow-up: Changed 9 years ago by ohanar

  • Description modified (diff)

I've created a new spkg that builds the cremona_mini database from Cremona's original text file. This requires a patch to the Sage root repository to reflect the new dependencies. I'm going to keep the optional spkg the same since it takes >5mins to build the database.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by leif

Replying to ohanar:

I've created a new spkg that builds the cremona_mini database from Cremona's original text file. This requires a patch to the Sage root repository to reflect the new dependencies.

Cool, thanks.

I'm going to keep the optional spkg the same since it takes >5mins to build the database.

Optional is optional; I'm not aware of any size demands or restrictions there...

comment:13 in reply to: ↑ 12 Changed 9 years ago by ohanar

Replying to leif:

Replying to ohanar:

I'm going to keep the optional spkg the same since it takes >5mins to build the database.

Optional is optional; I'm not aware of any size demands or restrictions there...

True, but I realized when updating the spkg that creating the database from text files more closely follows the spkg design of building libraries from source.

comment:14 follow-up: Changed 9 years ago by cremona

Many thanks for your work on this, Andrew. It would be good if it were very easy to extend further (say just by having more data files in the right place on installation), since I have not stopped and in fact am very close to adding the range 180k=190k. Adding that data (and more) should now be just a matter of adding some data files to the spkg, right? Is the information such as the maximum conductor hard-wired into the code or just read from the database after construction from raw data files?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 9 years ago by ohanar

Replying to cremona:

It would be good if it were very easy to extend further (say just by having more data files in the right place on installation), since I have not stopped and in fact am very close to adding the range 180k=190k. Adding that data (and more) should now be just a matter of adding some data files to the spkg, right?

Currently the spkg includes basically a single file -- a prebuilt database. This is primarily because it is much faster to have the package copy the file, rather than build the database from scratch (I was going to use time it takes to create it on sage.math as an example, but I got too impatient to let it finish). The script to create this database is sage.databases.cremona.build, this will build and install the database from your tgz file.

If we decide to, we can move the build script to be inside the spkg, with the knowledge that we should not use it on slow filesystems. If we do so, we should only include allcurves, allgens, allbsd, and degphi since those are the only files sage currently uses.

Is the information such as the maximum conductor hard-wired into the code or just read from the database after construction from raw data files?

There is still a bit of hard-wired code: a bit in the documentation, the build script, and an error message. It is possible to remove all of these references, the only reason I didn't when working on this patch was because I wanted to get sage up to speed. It is a simple fix, although if we decide remove the build scripts from the sage library to the spkg instead, we might as well do this at the same time.

comment:16 in reply to: ↑ 15 Changed 9 years ago by cremona

Replying to ohanar:

Replying to cremona:

It would be good if it were very easy to extend further (say just by having more data files in the right place on installation), since I have not stopped and in fact am very close to adding the range 180k=190k. Adding that data (and more) should now be just a matter of adding some data files to the spkg, right?

Currently the spkg includes basically a single file -- a prebuilt database. This is primarily because it is much faster to have the package copy the file, rather than build the database from scratch (I was going to use time it takes to create it on sage.math as an example, but I got too impatient to let it finish). The script to create this database is sage.databases.cremona.build, this will build and install the database from your tgz file.

If we decide to, we can move the build script to be inside the spkg, with the knowledge that we should not use it on slow filesystems. If we do so, we should only include allcurves, allgens, allbsd, and degphi since those are the only files sage currently uses.

Is the information such as the maximum conductor hard-wired into the code or just read from the database after construction from raw data files?

There is still a bit of hard-wired code: a bit in the documentation, the build script, and an error message. It is possible to remove all of these references, the only reason I didn't when working on this patch was because I wanted to get sage up to speed. It is a simple fix, although if we decide remove the build scripts from the sage library to the spkg instead, we might as well do this at the same time.

Thanks for the explanation. I agree that it is good to have the database file itself in the package and not the raw data, as long as it (the spkg) also has instructions on how to (re)create this from my raw data. And a note of the precise things to be changed when the range is extended would also be a good idea, even if that's only a couple of places. I'm thinking that it may not be you doing this next time!

comment:17 follow-up: Changed 9 years ago by ohanar

If 190k is almost done, I can add the upgrading instructions then.

comment:18 Changed 9 years ago by leif

I get warnings from Sphinx:

.../local/lib/python2.6/site-packages/sage/databases/cremona.py:docstring of sage.databases.cremona:26: (WARNING/2) Definition list ends without a blank line; unexpected unindent.
.../local/lib/python2.6/site-packages/sage/databases/cremona.py:docstring of sage.databases.cremona:36: (WARNING/2) Definition list ends without a blank line; unexpected unindent.

comment:19 follow-up: Changed 9 years ago by leif

I'm not sure to which ticket this belongs, but I get an apparently trivial doctest error in the following example of doc/en/bordeaux_2008/elliptic_curves.rst:

To use the database, just create a curve by giving

::

    sage: EllipticCurve('5077a1')
    Elliptic Curve defined by y^2 + y = x^3 - 7*x + 6 over Rational Field
    sage: C = CremonaDatabase() 
    sage: C.number_of_curves() #optional - cremona
    847550
    sage: C[37]
    {'a': {'a1': [[0, 0, 1, -1, 0], 1, 1],
           'b1': [[0, 1, 1, -23, -50], 0, 3], ...
    sage: C.isogeny_class('37b')
    [Elliptic Curve defined by y^2 + y = x^3 + x^2 - 23*x - 50 
    over Rational Field, ...]

File ".../doc/en/bordeaux_2008/elliptic_curves.rst", line 22:
    sage: C[37]
Expected:
    {'a': {'a1': [[0, 0, 1, -1, 0], 1, 1],
           'b1': [[0, 1, 1, -23, -50], 0, 3], ...
Got:
    {'allcurves': {'a1': [[0, 0, 1, -1, 0], 1, 1], 'b1': [[0, 1, 1, -23, -50], 0, 3], 'b2': [[0, 1, 1, -1873, -31833], 0, 1], 'b3': [[0, 1, 1, -3, 1], 0, 3]}}

(Just the first key differs; a vs. allcurves.)

comment:20 in reply to: ↑ 19 ; follow-up: Changed 9 years ago by cremona

  • Status changed from positive_review to needs_work

Replying to leif:

I'm not sure to which ticket this belongs, but I get an apparently trivial doctest error in the following example of doc/en/bordeaux_2008/elliptic_curves.rst:

(Just the first key differs; a vs. allcurves.)

I'm sure this is the correct ticket since the database has changed. It's easy to forget to test this part of the documentation. Changing the expected output to the actual output would be fine. Hence back to "Needs (trivial) work".

comment:21 in reply to: ↑ 20 Changed 9 years ago by ohanar

Replying to cremona:

Replying to leif:

I'm not sure to which ticket this belongs, but I get an apparently trivial doctest error in the following example of doc/en/bordeaux_2008/elliptic_curves.rst:

(Just the first key differs; a vs. allcurves.)

I'm sure this is the correct ticket since the database has changed. It's easy to forget to test this part of the documentation. Changing the expected output to the actual output would be fine.

Yup, didn't test this, so a good catch (and yes, it does belong here, I made the change in key names). Also, if you run the optional test, that will fail as well (we may want to scrap it for something else, since it adds to the maintenance of the optional spkg).

comment:22 in reply to: ↑ 17 Changed 9 years ago by cremona

Replying to ohanar:

If 190k is almost done, I can add the upgrading instructions then.

It's almost done in the sense that I have everything except that 3 curves in the range are "missing" on account of some difficulties with modular symbols which I will now try to fix (requires revising code and testing). Either I'll have this by the end of Thursday, or I'll be away for a week and it will take longer.

Changed 9 years ago by ohanar

Apply on top to sage library. Fixes issues with documentation.

comment:23 Changed 9 years ago by ohanar

  • Description modified (diff)

comment:24 Changed 9 years ago by ohanar

  • Status changed from needs_work to needs_review

comment:25 follow-up: Changed 9 years ago by cremona

Applying trac_11587.patch onto 4.7.2.alpha1 failed. And before that, installing the new spkg using sage -i ended with

...
Thread model: posix
gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5) 
****************************************************
/home/jec/sage-4.7.2.alpha1.ecdb/local/bin/sage-sage: line 807: 12145 Killed                  tee -a ../install.log "$SAGE_LOGS/$PKG_NAME".log

which looks like an error message, I am not sure. Am I doing something wrong?

comment:26 in reply to: ↑ 25 ; follow-up: Changed 9 years ago by leif

Replying to cremona:

Applying trac_11587.patch onto 4.7.2.alpha1 failed.

Did you first apply #11640 and #11642?

And before that, installing the new spkg using sage -i ended with

...
Thread model: posix
gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5) 
****************************************************
/home/jec/sage-4.7.2.alpha1.ecdb/local/bin/sage-sage: line 807: 12145 Killed                  tee -a ../install.log "$SAGE_LOGS/$PKG_NAME".log

which looks like an error message, I am not sure.

Yes. No idea what might have caused that.

comment:27 in reply to: ↑ 26 Changed 9 years ago by cremona

Replying to leif:

Replying to cremona:

Applying trac_11587.patch onto 4.7.2.alpha1 failed.

Did you first apply #11640 and #11642?

No since I saw that they were merged already -- but now I see that they were merged in alpha3 only.

comment:28 Changed 9 years ago by cremona

OK, with #11640 and #11642 applied first the patches to the Sage library apply fine. Dor the deps one I just manually edited the file since I could find no trace of mercurial in $SAGE_ROOT/spkgs/standard (or at least hg gave an error message). Then I rebuilt everything with sage -ba, and I am now doing a make ptestlong, and will report back.

Small comment, since I know that Andrew will be updating the conductor ranges anyway: at the moment smallest_conductor() returns 1 which will surprise some people as the actual smallest is 11!

comment:29 follow-up: Changed 9 years ago by cremona

  • Status changed from needs_review to positive_review

All test pass, so I am giving this a positive review. This is for the default database (max conductor 10000); but this ticket cannot be merged until the large database is also ready to go and has been tested.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 9 years ago by leif

Replying to cremona:

All test pass, so I am giving this a positive review.

I didn't expect something else. ;-)

This is for the default database (max conductor 10000); but this ticket cannot be merged until the large database is also ready to go and has been tested.

Which will be when?

Can't we just merge this ticket and open a follow-up in case something goes wrong with the larger (anyway optional) database?

comment:31 Changed 9 years ago by leif

P.S.: deps is part of the "root" repository, which is located in $SAGE_ROOT, so you have to apply the patch there.

comment:32 in reply to: ↑ 30 ; follow-up: Changed 9 years ago by cremona

Replying to leif:

Replying to cremona:

All test pass, so I am giving this a positive review.

I didn't expect something else. ;-)

This is for the default database (max conductor 10000); but this ticket cannot be merged until the large database is also ready to go and has been tested.

Which will be when?

I finished creating the data this morning, and Andrew Ohana is updating his optional spkg (?) now.

Can't we just merge this ticket and open a follow-up in case something goes wrong with the larger (anyway optional) database?

No, I don't think so, since the database has completely changed and so the *old* version of the optional larger database spkg will not be able to be applied to the *new* standard spkg. (Andrew, please correct me if I am wrong on this).

Anyway, the impression I got was that adding the additional curves published today would only take a few minutes. If longer, then go with what is on this ticket. But I don't know the right mechanism for changing the optional spkg.

comment:33 in reply to: ↑ 32 Changed 9 years ago by ohanar

Replying to cremona:

Replying to leif:

Replying to cremona:

This is for the default database (max conductor 10000); but this ticket cannot be merged until the large database is also ready to go and has been tested.

Which will be when?

I finished creating the data this morning, and Andrew Ohana is updating his optional spkg (?) now.

Working on it right now.

Can't we just merge this ticket and open a follow-up in case something goes wrong with the larger (anyway optional) database?

No, I don't think so, since the database has completely changed and so the *old* version of the optional larger database spkg will not be able to be applied to the *new* standard spkg. (Andrew, please correct me if I am wrong on this).

The optional spkgs mentioned the description work with the new database format, but there are a few places where the version of the package is hardcoded into the sage library (so holding off may be a good idea). I am going to remove these, so that maintaining the spkg is an easier chore.

Changed 9 years ago by ohanar

removes the hardcoded version number from sage library

comment:34 Changed 9 years ago by ohanar

  • Description modified (diff)

Ok, I've updated the spkg, it now includes updating instructions in the spkg.txt as well. I've also added a patch that removes the hardcoded version numbers from the sage library, the instructions in the spkg are based off of this.

comment:35 follow-up: Changed 9 years ago by leif

Andrew, is the optional spkg http://wstein.org/home/ohanar/cremona-database/database_cremona_ellcurve-20110915.spkg with the larger database ready for "release"?

comment:36 in reply to: ↑ 35 Changed 9 years ago by ohanar

Replying to leif:

Andrew, is the optional spkg http://wstein.org/home/ohanar/cremona-database/database_cremona_ellcurve-20110915.spkg with the larger database ready for "release"?

It should be (the install script simply copies a single file)

comment:37 Changed 9 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:38 Changed 9 years ago by leif

  • Description modified (diff)

Had to add a changelog entry for the previous version in Sage (0.1), since otherwise Jeroen's merger3 (meanwhile) rejects it (assuming it wasn't based on the previous one in Sage).

Corrected spkg at new location.

Changed 9 years ago by leif

"Proper" Mercurial changeset replacement patch. (Adds just the "User" field.)

comment:39 Changed 9 years ago by leif

  • Description modified (diff)

I've attached one replacement patch which just adds the "# User ..." line the original patch is lacking.

comment:40 Changed 9 years ago by cremona

After installing the optionl daatbase on top of 4.7.2.alpha3 I had a doctest failure in devel/sage/doc/en/bordeaux_2008/elliptic_curves.rst . Nothing serious, just different gens for a curve (i.e. the ones in the database are not the same as those found, for a rank 2 curve).

comment:41 follow-ups: Changed 9 years ago by jdemeyer

  • Cc davidloeffler added
  • Merged in sage-4.7.2.alpha3 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

This is the cause of a serious speed regression. Message by Dave Loeffler:

I've found the culprit: this is a side-effect of #11587. If you look at trac_11587.patch, then it removes a couple of lines of code at lines 710-712 of sage/databases/cremona.py which sets the generators to [] if the gens aren't explicitly in the database but the rank is known to be 0. The new version doesn't perform this little check and so we waste loads of time doing pointless 2-descents.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 9 years ago by leif

Replying to jdemeyer:

This is the cause of a serious speed regression. Message by Dave Loeffler:

I've found the culprit: this is a side-effect of #11587. If you look at trac_11587.patch, then it removes a couple of lines of code at lines 710-712 of sage/databases/cremona.py which sets the generators to [] if the gens aren't explicitly in the database but the rank is known to be 0. The new version doesn't perform this little check and so we waste loads of time doing pointless 2-descents.

I was fearing something like that.

Why not open a follow-up (blocker) ticket for fixing that?

alpha3 is already out...

comment:43 in reply to: ↑ 42 Changed 9 years ago by jdemeyer

Replying to leif:

Why not open a follow-up (blocker) ticket for fixing that?

I thought it might be easier to change an existing ticket, especially if the change is small.

alpha3 is already out...

Doesn't really matter, I can re-merge the ticket in alpha4...

comment:44 in reply to: ↑ 41 Changed 9 years ago by cremona

Replying to jdemeyer:

This is the cause of a serious speed regression. Message by Dave Loeffler:

I've found the culprit: this is a side-effect of #11587. If you look at trac_11587.patch, then it removes a couple of lines of code at lines 710-712 of sage/databases/cremona.py which sets the generators to [] if the gens aren't explicitly in the database but the rank is known to be 0. The new version doesn't perform this little check and so we waste loads of time doing pointless 2-descents.

It has always been rather a nuisance that the small default database differs from the large one not only in having fewer curves but in having less information about them (specifically, omits the known rank and generators). I don't want to delay this ticket longer than necessary (and apologise for the positive review which missed this glitch) but propose a follow-up which includes all known data for the small database too.

comment:45 Changed 9 years ago by jdemeyer

Follow-up or on this ticket will not make a difference because I plan to unmerge this ticket anyway if it causes such massive slowdown. So, do as you wish.

comment:46 follow-up: Changed 9 years ago by jdemeyer

  • Milestone changed from sage-4.7.2 to sage-4.7.3

This will be unmerged for the sage-4.7.2 release series. We really should get this working so that it can be merged again in sage-4.7.3.alpha0.

comment:47 in reply to: ↑ 46 Changed 9 years ago by leif

Replying to jdemeyer:

This will be unmerged for the sage-4.7.2 release series. We really should get this working so that it can be merged again in sage-4.7.3.alpha0.

Then we should also remove http://sagemath.org/packages/optional/database_cremona_ellcurve-20110915.spkg again, since it is in the new format (which only the alpha3 supports); either when alpha4 is officially released, or right before the final 4.7.2 gets out.

It shouldn't be hard to change its spkg-install to just install the previous version (hard-coded URL, e.g. by recursively calling sage -i ... or sage-spkg) if the Sage version present doesn't support the new format, which is reasonable for the future anyway (i.e., when this ticket gets remerged). Cf. sage-release.

comment:48 Changed 9 years ago by leif

... or simply print a message instructing the user to instead do

$ ./sage -i http://sagemath.org/packages/archive/database_cremona_ellcurve-20071019.p0.spkg

(It should IMHO also still be in /packages/optional/, along with its description, but I get a 404 if I replace archive by optional above.)


Ceterum censeo we should really split the spkg metadata (or Sage part) from its "real" contents (data or upstream tarballs), or at least additionally provide it in small files to be processed without the need to download the full spkgs in advance...

comment:49 follow-up: Changed 9 years ago by cremona

Any effort in removing this upgrade which takes more than the few minutes it would take to fix the very minor problem in it would be a complete waste of time.

comment:50 in reply to: ↑ 49 ; follow-up: Changed 9 years ago by leif

Replying to cremona:

Any effort in removing this upgrade which takes more than the few minutes it would take to fix the very minor problem in it would be a complete waste of time.

I fully agree, but unfortunately it's relatively easy to "unmerge" this ticket, and it's in general not up to the release manager to fix such things (that go beyond simple rebasing, fixing noise, permissions or commit messages etc., which IMHO at least partially already is a voluntary service).

comment:51 in reply to: ↑ 50 Changed 9 years ago by cremona

  • Description modified (diff)

Replying to leif:

Replying to cremona:

Any effort in removing this upgrade which takes more than the few minutes it would take to fix the very minor problem in it would be a complete waste of time.

I fully agree, but unfortunately it's relatively easy to "unmerge" this ticket, and it's in general not up to the release manager to fix such things (that go beyond simple rebasing, fixing noise, permissions or commit messages etc., which IMHO at least partially already is a voluntary service).

Of course -- I just meant that you should do the absolute minimum and not spend effort making the old system more clever. And I just have not had time to fix the patch.

Changed 9 years ago by davidloeffler

apply over previous patches

comment:52 follow-up: Changed 9 years ago by davidloeffler

  • Description modified (diff)
  • Status changed from new to needs_review

comment:53 in reply to: ↑ 52 Changed 9 years ago by cremona

Replying to davidloeffler:

Not tested but David's patch looks perfect and I believe his comment. Better if someone else test this as David and I use the same machine.

If this is not merged soon it will become out of date since I have currently reached 198800 and expect to release an extension up to 200k within a week!

comment:54 in reply to: ↑ 41 ; follow-ups: Changed 9 years ago by ohanar

  • Status changed from needs_review to positive_review

Everything works as expected.

@davidloeffler I don't think that those lines of code were actually ever used in the old version, since the small database actually included the generators despite the documentation saying otherwise (the new version follows what the documentation specified).

@cremona I've posted a new spkg with your expanded tables, same location as before.

comment:55 in reply to: ↑ 54 Changed 9 years ago by cremona

Replying to ohanar:

Everything works as expected.

@davidloeffler I don't think that those lines of code were actually ever used in the old version, since the small database actually included the generators despite the documentation saying otherwise (the new version follows what the documentation specified).

@cremona I've posted a new spkg with your expanded tables, same location as before.

Many thanks to all. As predicted, the database was updated to go up to 200k a few days ago.

comment:56 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.7.3.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:57 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:58 Changed 9 years ago by cremona

  • Description modified (diff)

I just updated the description to reflect the fact that the optional db now goes up to 200k.

comment:59 in reply to: ↑ 54 Changed 9 years ago by jdemeyer

  • Description modified (diff)

Replying to ohanar:

I've posted a new spkg with your expanded tables, same location as before.

Not really same location as before, I updated the ticket description.

comment:60 Changed 9 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:61 Changed 9 years ago by jdemeyer

  • Merged in changed from sage-4.7.3.alpha0 to sage-4.8.alpha0
  • Milestone set to sage-4.8

comment:62 follow-up: Changed 9 years ago by ohanar

I've posted an updated spkg based on Cremona's update to 210000 (http://wstein.org/home/ohanar/cremona-database/database_cremona_ellcurve-20111121.spkg).

I noticed that I never put in a way to install the old spkg for old copies of sage, so I added a recursive sage -i for now, until we have a standard that frees the user from downloading two different versions of an spkg. It should work for any version of sage released within the last year, although I haven't tested it super thoroughly (it works with stable 4.7.x, 4.8.alpha3, and 4.3.3.alpha0).

comment:63 in reply to: ↑ 62 ; follow-up: Changed 9 years ago by jdemeyer

Replying to ohanar:

I've posted an updated spkg based on Cremona's update to 210000 (http://wstein.org/home/ohanar/cremona-database/database_cremona_ellcurve-20111121.spkg).

Please make a new ticket for making this an optional spkg (this ticket is closed).

comment:64 in reply to: ↑ 63 Changed 9 years ago by ohanar

Replying to jdemeyer:

Please make a new ticket for making this an optional spkg (this ticket is closed).

Sure, my only reason for posting it in this ticket was due to the unaddressed issue of dealing with old copies of Sage.

comment:65 follow-up: Changed 9 years ago by cremona

Doing a full test after installing the new optional database (onto 4.8.alpha4) I find one doctest failure in devel/sage/doc/en/bordeaux_2008/elliptic_curves.rst:

    sage: E = EllipticCurve([1,2,5,7,17])
    sage: E.conductor()       # not in the Tables
    154907
    sage: E.gens()            # a few seconds
    [(1 : 3 : 1), (67/4 : 507/8 : 1)]

since that curve is now in the database and the listed generators are different. I suggest instead

sage: E = EllipticCurve([0,0,1,-19,60])
sage: E.conductor()        # not in the Tables
1129211
sage: E.gens()             # a few seconds
[(-5 : 5 : 1), (-4 : 8 : 1), (-15/4 : 67/8 : 1), (-3 : 9 : 1)]

Should the ticket be reopened -- this is not an issue in testing without the optional db installed?

comment:66 in reply to: ↑ 65 Changed 9 years ago by cremona

Replying to cremona:

Should the ticket be reopened -- this is not an issue in testing without the optional db installed?

See #12184

Note: See TracTickets for help on using tickets.