Making staNMF pip installable & Adding optional keyword arguments to runNMF

[amyecampbell]

Packaged code and wrote setup.py Added LICENSE.txt and README.txt Added optional keyword parameters to runNMF() function using **kwargs (allowing users to change parameters entered into SPAMS train function)

[dhimmel]

Unrecognized author. If this is you, make sure amyecampbell@gmail.com is associated with your account. Click to add email addresses in your account settings.

Make sure GitHub knows about your email

[dhimmel]

I'd recommend placing your LICENSE file in the root of this repo. Basically, I consider GitHub rather than pypi the authoritative home for the project. If you want, you can point the license argument in staNMF/setup.py to the path ../LICENSE (I think).

[dhimmel]

I think the standard method is to put setup.py in the root of your repository. Here are some examples:

• https://github.com/migonzalvar/pysagec/blob/master/setup.py
• https://github.com/cvxgrp/cvxpy/blob/master/setup.py
• https://github.com/iesl/OpenReviewPy/blob/master/setup.py
• https://github.com/Horta/ncephes/blob/master/setup.py

[dhimmel]

Once these comments are addressed, I will perform a testing review: by attempting a clean pip install in a python docker.

[dhimmel]

How does spams get installed? It doesn't seem to be on pypi?

Is this the only website for the package: http://spams-devel.gforge.inria.fr/index.html? In other words, it's not on a repository hosting service? In my experience, my experiences with packages that do not use a repository hosting service have generally been negative.

[amyecampbell]

re: email I agree, I should change it to the email associated with my github account

re: License Yeah, i'll try having it point to that and will see if that works

re: setup in the root, I think the thing here is that a whole folder(staNMF) is contained within the root staNMF folder; this is I think just the product of how I originally built my directories when I was writing the code-- would it make sense to just put everything in the root (keeping the data folder its own folder within) ? That'd also put the readme and license files in the main folder.

re: SPAMS. Yeah I used this package because it was what the authors of the original paper used (though they used the matlab version). It's definitely a problem that it's not pip installable, but it also requires a single, two-dimensional input 'guess' matrix, which is important for calculating instability

[dhimmel]

Check out the directory structure of the repos I linked to.

There is a folder with the same name as the package but it only contains source code.

[dhimmel]

The .egg-info directory should not be tracked. In future, I recommend only adding files explicitly. Unless you know what a file is and know it should be tracked, don't add. But you should also add *.egg-info/ to your .gitignore.

[dhimmel]

Also dist/ should not be tracked.

[dhimmel]

Remove README.md since you want README.rst to show up on GitHub?

[dhimmel]

.pyc file should not be tracked. You should make a .gitignore file to prevent these issues.

[dhimmel]

Tried to install in a python 2.7 Docker using:

pip install git+https://github.com/amyecampbell/staNMF.git

Got the following error:

    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/tmp/pip-cTDX5O-build/setup.py", line 5, in <module>
        with open(os.path.join(os.path.dirname(__file__), 'README.txt')) as readme:
    IOError: [Errno 2] No such file or directory: '/tmp/pip-cTDX5O-build/README.txt'

[amyecampbell]

Ohhh yeah it's because I changed the readme to .rst. I'll update that

[dhimmel]

You still need to forget about these ignored files. See http://stackoverflow.com/a/19095988/4651668

[dhimmel]

The last commit titled "removing ignored files" did not remove ignored files. Also .Rhistory should be ignored (:

[dhimmel]

Getting an installation error:

Collecting spams (from staNMF==0.1)
  Could not find a version that satisfies the requirement spams (from staNMF==0.1) (from versions: )
No matching distribution found for spams (from staNMF==0.1)

I think the issue is that find_packages (doc) is finding spams and trying to install it from pypi where it doesn't exist. You could specify exclude=('spams'), but there is the bigger issue of how are people going to install spams?

[dhimmel]

I'm having trouble installing spams via:

pip install http://spams-devel.gforge.inria.fr/hitcounter2.php?file=33816/spams-python-v2.5-svn2014-07-04.tar.gz

Getting an error:

Command "/usr/local/bin/python2 -u -c "import setuptools, tokenize;__file__='/tmp/pip-_Od0LE-build/setup.py';exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\r\n', '\n'), __file__, 'exec'))" install --record /tmp/pip-VoWG8n-record/install-record.txt --single-version-externally-managed --compile" failed with error code 1 in /tmp/pip-_Od0LE-build/

I see a few options for this pull request:

• allowing users to deal with the horrors of spam installation themselves
• find a scripted method for installing spams that works
• include spams source code in your repo (it's GPLv3)
• purge spams

@cgreene what do you think?

[amyecampbell]

yeah, that's almost certainly the issue. would i specify that within findpackages() in setup.py?

And yes, I specify in the documentation that you need to download and install spams from the group's webpage (and where to find it). Is there a way (and is it okay/customary) to include SPAMS in its own folder in the package as long as the fact that I've done that ends up in the README and license file?

[cgreene]

I'd just allow users to deal with the horror.

[dhimmel]

would i specify that within findpackages() in setup.py?

Yep, as explained in the find_packages docs I linked to.

[dhimmel]

I'd just allow users to deal with the horror.

Okay but I'm not going to fully test the installation because I don't want to deal with installing spams and it's dependencies.

I'm also uploading spams-python-v2.5-svn2014-07-04.tar.gz to GitHub because I'm worried that the longevity of the spams website.

@amyecampbell should you specify the spams version in README?

[amyecampbell]

Yeah, I should!

[dhimmel]

You need to create an empty file at staNMP/__init__.py. For why see http://stackoverflow.com/a/448279/4651668

[dhimmel]

The dependency_links argument of setuptools.setup seemed promising for installing spams, but I couldn't get it to work. So I now fully support "allowing users to deal with the horror" of installing spams.

That's the way I had it before, and I changed it (with Dongbo's help) to find_packages() because it kept throwing an error during installation

Probably because you didn't have staNMF/__init__.py file. Right now find_packages doesn't return any pacakges, so you're not actually installing anything.

You should make sure that you can open a python session outside of your staNMF directory and import staNMF. If not, the package hasn't been installed.

[amyecampbell]

Yeah, you're probably right. It does, as of now, work to import staNMF to a project outside of the staNMF directory. I haven't tested it having added init.py and with these changes to setup.py

[amyecampbell]

Ok, so the actual installation step seems to work with the updates, and it is importable from different directories

[dhimmel]

Looks good to me! Nice work overcoming the terrors and poor doc of setuptools!

[dhimmel]

Confirming that I successfully installed via

pip install git+https://github.com/amyecampbell/staNMF.git

in the Python 2.7 Docker.

Note that import staNMF.staNMF failed as expected due to missing spams dependency.

[gwaybio]

I have nothing to add, nice work!