Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update installation instructions #533

Merged
merged 5 commits into from
Aug 19, 2018
Merged

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Aug 13, 2018

  • Closes improve installation docs #531
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

I built the documentation locally and it looked good to me.

@wholmgren wholmgren added this to the 0.6.0 milestone Aug 13, 2018
# get the package from the conda-forge conda channel
conda install -c conda-forge pvlib-python

# get the package from the Python Package Index
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using --no-deps if you are using anaconda, otherwise you may get strange results. - In fact why not move this to an "Advanced User" section, b/c for the most part users who don't know what to do should only use Anaconda, and users who know how to use pip don't need installation instructions.

Also, a nit, I recommend creating a new conda env if using conda-forge, IMO you should not use conda-forge in your anaconda base env, so that leaves only the pvlib channel option which should work for most users. Maybe move conda-forge to advanced users section. If you use conda-forge in your base Anaconda, conda-forge has a tendency to take over, and so IMO it should probably be the default channel in any conda env it's used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the good feedback.

In my experience, pip install pvlib within an environment with sufficient numpy/pandas versions will not do anything but install pvlib itself. Is it safe to assume that people are using an Anaconda distribution that is recent enough (or updated enough) to meet the modest minimum numpy/pandas requirements? I would say yes, but perhaps you've seen otherwise. If it is safe for pvlib, then I don't think --no-deps helps and it makes it looks unnecessarily complicated.

The "Install as an editable library" section is, I think, largely what an "Advanced User" section would look like. I think the existing name is preferable because a relatively large percentage of people that are new to pvlib are also people that would want to edit the source code. Maybe I'm wrong about that and/or maybe that was more true a few years ago than now.

What about adding a second comment line for each install option such as:

    # get the package from the pvlib conda channel
    # best option for installing in the base Anaconda distribution
    conda install -c pvlib pvlib

    # get the package from the conda-forge conda channel
    # best option if using forecast module. consider a separate conda env
    conda install -c conda-forge pvlib-python

    # get the package from the Python Package Index
    # best option if you know what you are doing
    pip install pvlib

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My view is that the installation should only give one very easy, full-proof method. Unfortunately that's hard b/c there are so many things that can go wrong, and even a slight hurdle can discourage many users (you'd be surprised!). That's why I feel that only the first option for install should be here, very close to the top, with lots of white space, and less text. Something like a "quick start guide"

Getting Started

To install pvlib into an existing Anaconda Python base/root environment just use conda:

(base)$ conda install -c pvlib pvlib

Note: on some older Anaconda installations, the base environment is called root.

Everything else can go in a separate section. EG: I would not recommend using conda-forge in your base/root anaconda environment, which means that they now have to create a conda-env which might be hurdle for them, and there are several things that could go wrong. And if they are in a conda-env, or even in the anaconda base/root env, or if they use miniconda, or if they just haven't updated in a while, and they use pip install pvlib then it could overwrite their numpy with openblas instead of MKL and then that be too much for them. Like I said earlier, if they are advanced enough to use pip, then adding --no-deps isn't an issue for them, and if not, then IMO they should not probably not use pip.

Perhaps we should get some more feedback from others?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the second comment you added, maybe a good compromise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the intro paragraph to push people to conda and I added the comments. Happy to continue iterating since this is important.

My typical workflow includes pip-installing pvlib in conda envs that already have numpy and pandas (or more). I've not had any issues with this and I would be surprised if any other users have. But it does require care to mix together pip and conda installs, so I agree that we should dissuade people from doing it.

* pytables: Linke turbidity look up for clear sky models
* numba: fastest solar position calculations
* pyephem: solar positions calculations using an astronomical library
* siphon: forecasting PV power using the pvlib.forecast module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO having a requirements.txt file makes specifying requirements and letting users create non-conda virtual environments easier, here's my requirements.txt for pvlib in linux (not conda):

# build and run
pytz
numpy
scipy
pandas
numba
ephem
siphon
netcdf4
tables
# docs and testing
matplotlib
sphinx
sphinx_rtd_theme
ipython
jupyter
numpydoc
pytest
pytest-cov
pytest-mock
nose
nbsphinx

some of these are not available from either anaconda.org or conda-forge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am open to

  1. adding a requirements file or files, and/or
  2. adding support for e.g. pip install pvlib[all]

in

  1. this pull request, or
  2. a separate pull request

Not sure what the best approach is, though, which makes me think defer each of those items to their own PRs. They are certainly related to this topic, though, especially if the result of the "Advanced User" or similar request involves a larger restructuring.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed let's consider requirements.txt in a separate PR, but I was just noting that there are more dependencies than just numpy and pandas.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional dependencies ;)

@wholmgren
Copy link
Member Author

merging this one now, but @mikofski feel free open new issues/PRs with the additional changes you'd like to see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants