ihesp / IPART

Image-Process based Atmospheric River Tracking (IPART) algorithms
https://ipart.readthedocs.io/en/latest/
GNU General Public License v3.0
23 stars 8 forks source link

Corrections to the installation instructions in docs & README #4

Closed sadielbartholomew closed 4 years ago

sadielbartholomew commented 4 years ago

The installation instructions are largely fine, but I have noticed a few issues whilst conducting my review for openjournals/joss-reviews#2407, & it is more efficient for me to correct these as suggestions via a Pull Request than opening an Issue to try to explain them. Namely:

  1. the README instructions finish with a choice of two commands that finally allow the installation & only one such command works for me (see detail below) & I think only that same one will work generally for any user;
  2. the conda create commands in the docs under 'Install from conda (experimental)' are both broken due to the syntax but work if one component is moved (see below);
  3. there's a point in the text where it says to 'see above' for dependencies when they are listed subsequently rather than previously (a minor point but it would be good to point readers in the right direction so I suggest this is re-worded).

Detail on (1)

Following the conda installation commands exactly as in the README, I am able to successfully install IPART and (it then runs the tests etc. as working) with pip install -e ., but the alternative provided, python setup.py develop, does not work for me using either the Python 2 or 3 environments set up according to the previous commands. When I run it I hit the error:

$ python setup.py develop
running develop
running egg_info
creating ipart.egg-info
writing ipart.egg-info/PKG-INFO
writing dependency_links to ipart.egg-info/dependency_links.txt
writing requirements to ipart.egg-info/requires.txt
writing top-level names to ipart.egg-info/top_level.txt
writing manifest file 'ipart.egg-info/SOURCES.txt'
reading manifest file 'ipart.egg-info/SOURCES.txt'
writing manifest file 'ipart.egg-info/SOURCES.txt'
running build_ext
Creating /home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/site-packages/ipart.egg-link (link to .)
Adding ipart 2.0 to easy-install.pth file

Installed /home/sadie/IPART
Processing dependencies for ipart==2.0
Searching for cdutil
Reading https://pypi.org/simple/cdutil/
Couldn't find index page for 'cdutil' (maybe misspelled?)
Scanning index of all packages (this may take a while)
Reading https://pypi.org/simple/
No local packages or working download links found for cdutil
error: Could not find suitable distribution for Requirement.parse('cdutil')

(the same error arises in both the Python 2 and 3 environment cases). Something about the cdutil package means that the command is not sufficient. There may be a way to adapt it to make it work but since pip install -e . works fine anyway I suggest in this PR to just state to use that instead. Though feel free to address this issue in another way if you wish (in which case I can remove it from the PR).

Detail on (3)

Note the conda error raised for the first such command as-is, fixed when the ipart package specification is moved before the -c. (It may be that the current commands work on certain conda versions, perhaps that you were using when testing these commands, but I believe the working example here is the standard syntax that should be guaranteed to work across any version & is therefore safer):

$ conda create -n ipartpy2 python=2.7 -c guangzhi -c conda-forge ipart
usage: conda [-h] [-V] command ...
conda: error: unrecognized arguments: ipart
$ conda create -n ipartpy2 python=2.7 ipart -c guangzhi -c conda-forge
Collecting package metadata (current_repodata.json): done
Solving environment: failed with repodata from current_repodata.json, will retry with next repodata source.
Collecting package metadata (repodata.json): done
Solving environment: done

## Package Plan ##

  environment location: /home/sadie/anaconda3/envs/ipartpy20

  added / updated specs:
    - ipart
    - python=2.7

The following packages will be downloaded:

    package                    |            build
    ---------------------------|-----------------
    _libgcc_mutex-0.1          |      conda_forge           3 KB  conda-forge
    _openmp_mutex-4.5          |            0_gnu         435 KB  conda-forge
...
...
...
    zlib-1.2.11                |    h516909a_1006         105 KB  conda-forge
    zstd-1.4.5                 |       h6597ccf_1         428 KB  conda-forge
    ------------------------------------------------------------
                                           Total:       272.6 MB

The following NEW packages will be INSTALLED:

  _libgcc_mutex      conda-forge/linux-64::_libgcc_mutex-0.1-conda_forge
  _openmp_mutex      conda-forge/linux-64::_openmp_mutex-4.5-0_gnu
...
...
...
  zlib               conda-forge/linux-64::zlib-1.2.11-h516909a_1006
  zstd               conda-forge/linux-64::zstd-1.4.5-h6597ccf_1

Proceed ([y]/n)? n
Xunius commented 4 years ago

@sadielbartholomew Many thanks for the PR. Code distribution is something I'm really need to gain more knowledge about. The fixes look reasonable. I can also confirm that I made a conda syntax error regarding the 2nd point.