Opened 5 years ago
Closed 5 years ago
#21086 closed enhancement (fixed)
new style package for database_kohel
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  packages: optional  Keywords:  
Cc:  jdemeyer, was, vbraun  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  7df7bce (Commits, GitHub, GitLab)  Commit:  7df7bcee6f5ed97f01f8ccca8c50d8e7807cd709 
Dependencies:  Stopgaps: 
Description
Make a new style package for database_kohel
Upstream tarball available at
http://www.labri.fr/perso/vdelecro/database_kohel20160724.tar.gz
Change History (15)
comment:1 Changed 5 years ago by
 Branch set to u/vdelecroix/21086
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Commit set to e6902ed430e889bdc76106e898f9007d4a8ff346
Branch pushed to git repo; I updated commit sha1. New commits:
e6902ed  Trac 21086: database_kohel new style package

comment:3 Changed 5 years ago by
 Status changed from needs_review to needs_work
 You need to check for errors in
spkginstall
(in particular, themv
commands).
 You can replace
if [ d ${TARGET} ] then echo "REMOVING PREVIOUS VERSION" rm rf ${TARGET} fi
by
rm rf ${TARGET}
 I don't like this error handling:
try: coeff_list = _dbz_to_integer_list(modpol) except IOError: raise KeyError("No database entry for modular polynomial of level %s"%level)
I think it would be better to raise the error directly in _dbz_to_integer_list
and then only if the file really cannot be opened. If the error is raised while reading the file, it should be propagated as IOError
. I also think that KeyError
is not really appropriate, since it's not a key. I would prefer LookupError
.
comment:4 followup: ↓ 6 Changed 5 years ago by
I should also add that this package is not as important as it used to be, since PARI can now compute the modular polynomials provided by this package.
comment:5 Changed 5 years ago by
For the dependencies
, can you use a more explicit # no dependencies
?
comment:6 in reply to: ↑ 4 ; followup: ↓ 9 Changed 5 years ago by
Replying to jdemeyer:
I should also add that this package is not as important as it used to be, since PARI can now compute the modular polynomials provided by this package.
Could you provide the commands available to check the data in the package?
What do you think about using pari and deprecate the database?
comment:7 Changed 5 years ago by
 Commit changed from e6902ed430e889bdc76106e898f9007d4a8ff346 to 04e9d9d22e3288080a8f074bcf4814d69f9475c2
Branch pushed to git repo; I updated commit sha1. New commits:
04e9d9d  Trac 21086: review

comment:8 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:9 in reply to: ↑ 6 Changed 5 years ago by
Replying to vdelecroix:
Could you provide the commands available to check the data in the package?
Maybe you should ask the PARI developers (are you currently in Bordeaux?)
What do you think about using pari
This already happens. The database is no longer used by anything else in Sage.
and deprecate the database?
I don't think this is needed. It is still useful to avoid long computations if you need the large polynomials.
comment:10 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to needs_work
You didn't fix this:
 You need to check for errors in
spkginstall
(in particular, themv
commands).
comment:11 Changed 5 years ago by
Another detail (feel free to fix this or not): I don't think that the extra arguments to __getitem__
are useful:
def __getitem__(self, disc, level=1, var='x'):
I would change this to
def __getitem__(self, disc):
comment:12 Changed 5 years ago by
 Commit changed from 04e9d9d22e3288080a8f074bcf4814d69f9475c2 to 7df7bcee6f5ed97f01f8ccca8c50d8e7807cd709
comment:13 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:14 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:15 Changed 5 years ago by
 Branch changed from u/vdelecroix/21086 to 7df7bcee6f5ed97f01f8ccca8c50d8e7807cd709
 Resolution set to fixed
 Status changed from positive_review to closed
(few minutes needed to upload the tarball)