Opened 7 years ago
Closed 7 years ago
#21086 closed enhancement (fixed)
new style package for database_kohel
Reported by: | Vincent Delecroix | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.3 |
Component: | packages: optional | Keywords: | |
Cc: | Jeroen Demeyer, William Stein, Volker Braun | 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_kohel-20160724.tar.gz
Change History (15)
comment:1 Changed 7 years ago by
Branch: | → u/vdelecroix/21086 |
---|---|
Status: | new → needs_review |
comment:2 Changed 7 years ago by
Commit: | → e6902ed430e889bdc76106e898f9007d4a8ff346 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
e6902ed | Trac 21086: database_kohel new style package
|
comment:3 Changed 7 years ago by
Status: | needs_review → needs_work |
---|
- You need to check for errors in
spkg-install
(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 follow-up: 6 Changed 7 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 7 years ago by
For the dependencies
, can you use a more explicit # no dependencies
?
comment:6 follow-up: 9 Changed 7 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 7 years ago by
Commit: | e6902ed430e889bdc76106e898f9007d4a8ff346 → 04e9d9d22e3288080a8f074bcf4814d69f9475c2 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
04e9d9d | Trac 21086: review
|
comment:8 Changed 7 years ago by
Status: | needs_work → needs_review |
---|
comment:9 Changed 7 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 7 years ago by
Reviewers: | → Jeroen Demeyer |
---|---|
Status: | needs_review → needs_work |
You didn't fix this:
- You need to check for errors in
spkg-install
(in particular, themv
commands).
comment:11 Changed 7 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 7 years ago by
Commit: | 04e9d9d22e3288080a8f074bcf4814d69f9475c2 → 7df7bcee6f5ed97f01f8ccca8c50d8e7807cd709 |
---|
comment:13 Changed 7 years ago by
Status: | needs_work → needs_review |
---|
comment:14 Changed 7 years ago by
Status: | needs_review → positive_review |
---|
comment:15 Changed 7 years ago by
Branch: | u/vdelecroix/21086 → 7df7bcee6f5ed97f01f8ccca8c50d8e7807cd709 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
(few minutes needed to upload the tarball)