Opened 10 years ago

Closed 10 years ago

Last modified 5 years ago

#9223 closed enhancement (fixed)

improve doctest coverage of databases/cremona.py

Reported by: AlexGhitza Owned by: mvngu
Priority: minor Milestone: sage-4.5.2
Component: doctest coverage Keywords: cremona elliptic curve database
Cc: Merged in: sage-4.5.2.alpha1
Authors: Alex Ghitza, John Cremona Reviewers: John Cremona, Alex Ghitza
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by chapoton)

As of sage-4.4.3, we have:

ERROR: Please add a `TestSuite(s).run()` doctest.
SCORE cremona.py: 42% (17 of 40)

Missing documentation:
 * _init(self, ftpdata):
 * __repr__(self):
 * CremonaDatabase():

Missing doctests:
 * rebuild(data_tgz, largest_conductor, decompress=True):
 * __init__(self, read_only=True):
 * __iter__(self):
 * __getitem__(self, N):
 * __repr__(self):
 * allbsd(self, N):
 * allcurves(self, N):
 * allgens(self, N):
 * degphi(self, N):
 * elliptic_curve_from_ainvs(self, N, ainvs):
 * elliptic_curve(self, label):
 * iter(self, conductors):
 * isogeny_classes(self, conductor):
 * isogeny_class(self, label):
 * list(self, conductors):
 * _init_allcurves(self, ftpdata, largest_conductor=0):
 * _init_degphi(self, ftpdata, largest_conductor=0):
 * _init_allbsd(self, ftpdata, largest_conductor=0):
 * _init_allgens(self, ftpdata, largest_conductor=0):
 * __init__(self, read_only=True):

Attachments (2)

trac_9223.patch (16.8 KB) - added by AlexGhitza 10 years ago.
trac_9223-reviewer.patch (5.7 KB) - added by cremona 10 years ago.
Apply after previous patch

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by AlexGhitza

  • Authors set to Alex Ghitza

After the patch:

ERROR: Please add a `TestSuite(s).run()` doctest.
SCORE cremona.py: 85% (34 of 40)

Missing documentation:
	 * _init(self, ftpdata):


Missing doctests:
	 * rebuild(data_tgz, largest_conductor, decompress=True):
	 * _init_allcurves(self, ftpdata, largest_conductor=0):
	 * _init_degphi(self, ftpdata, largest_conductor=0):
	 * _init_allbsd(self, ftpdata, largest_conductor=0):
	 * _init_allgens(self, ftpdata, largest_conductor=0):

I'm not sure how to test the remaining ones...

Changed 10 years ago by AlexGhitza

comment:2 Changed 10 years ago by AlexGhitza

  • Status changed from new to needs_review

comment:3 Changed 10 years ago by cremona

I applied the patch successfully to a 4.4.4.alpha0 build, which did not at first have the optional database installed. I tested that the command to install it worked as advertised, and that all tests in sage/databases/cremona.py passed, with and without -optional. I noticed a real bug and some cosmetic stuff, and made a new patch (reviewer patch, to be applied after the main one). Then to test that I went to a different machine which did not have the optional database installed, applied both patches and tested the non-optional tests, then installed the optional database and tested both non-optional and non-optional tests. Phew!

Here's the thing I found of actual substance: in the iterator, the parameter should be a complete list of conductors, or a generator object, and *not* the first and last conductor wanted. So in the function and test as it was, the iterator delivers the three curves of conductor 11 and nothing else!

Finally, I agree that it is not reasonable to test the (re)-installation of the database in the normal way, since for a start it takes a long time. I think we need some input from William on this. Around 2006 I was updating the database regularly, and giving him access to the new tgz file, but since then it has happened only rarely. But it is very important that it still works!

Changed 10 years ago by cremona

Apply after previous patch

comment:4 Changed 10 years ago by cremona

PS Apart from that range issue, I give a positive review to everything else Alex did, so all we need is someone to review my additional patch, and a decision on what to do about the remaining missing tests. Can we tag them as "not tested" with some extra explanatory code? Of course, somce one other than William ought to test the rebuilding of the database, and it should probably be me as I am the only one who has the raw data.

comment:5 Changed 10 years ago by AlexGhitza

  • Authors changed from Alex Ghitza to Alex Ghitza, John Cremona
  • Reviewers set to John Cremona, Alex Ghitza
  • Status changed from needs_review to positive_review

John's reviewer patch looks good to me. Sorry about the iterator business: when I tested that method, I realised that it was broken but for some reason I put [11, blah] instead of the [11..blah] that I intended. Your revised tests for that also make more sense.

I agree that we want the database installation code to keep working, but I really don't know how/whether we can doctest this. It's an issue that concerns all the databases so it would be good to have a general solution. I'd like to ask about this on sage-devel, and if we can get a good method going it can be implemented on a new ticket.

In the meantime, it's better to get the things on this ticket going.

comment:6 Changed 10 years ago by ddrake

  • Merged in set to sage-4.5.2.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:7 Changed 5 years ago by chapoton

  • Description modified (diff)
Note: See TracTickets for help on using tickets.