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: sage-7.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:

Status badges

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 5 years ago by vdelecroix

  • Branch set to u/vdelecroix/21086
  • Status changed from new to needs_review

(few minutes needed to upload the tarball)

comment:2 Changed 5 years ago by git

  • Commit set to e6902ed430e889bdc76106e898f9007d4a8ff346

Branch pushed to git repo; I updated commit sha1. New commits:

e6902edTrac 21086: database_kohel new style package

comment:3 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. You need to check for errors in spkg-install (in particular, the mv commands).
  1. You can replace
    if [ -d ${TARGET} ]
    then
    	echo "REMOVING PREVIOUS VERSION"
        rm -rf ${TARGET}
    fi
    

by

rm -rf ${TARGET}
  1. 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: Changed 5 years ago by 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.

comment:5 Changed 5 years ago by jdemeyer

For the dependencies, can you use a more explicit # no dependencies?

comment:6 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by vdelecroix

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 git

  • Commit changed from e6902ed430e889bdc76106e898f9007d4a8ff346 to 04e9d9d22e3288080a8f074bcf4814d69f9475c2

Branch pushed to git repo; I updated commit sha1. New commits:

04e9d9dTrac 21086: review

comment:8 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

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

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 jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

You didn't fix this:

  1. You need to check for errors in spkg-install (in particular, the mv commands).

comment:11 Changed 5 years ago by jdemeyer

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 git

  • Commit changed from 04e9d9d22e3288080a8f074bcf4814d69f9475c2 to 7df7bcee6f5ed97f01f8ccca8c50d8e7807cd709

Branch pushed to git repo; I updated commit sha1. New commits:

aa84574Trac 21086: check for errors in spkg-install
7df7bceTrac 21086: remove keyword argument to __getitem__

comment:13 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:14 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:15 Changed 5 years ago by vbraun

  • Branch changed from u/vdelecroix/21086 to 7df7bcee6f5ed97f01f8ccca8c50d8e7807cd709
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.