-
Notifications
You must be signed in to change notification settings - Fork 1.1k
add extras_require complete and test options #566
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
Conversation
test
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
odd I don't see probably best to add it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add netcdf4
to complete extras_require
setup.py
Outdated
TESTS_REQUIRE = ['pytest', 'nose'] | ||
TESTS_REQUIRE = ['pytest', 'pytest-cov', 'pytest-mock', 'nose'] | ||
EXTRAS_REQUIRE = { | ||
'complete': ['scipy', 'tables', 'numba', 'siphon', 'ephem'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add netcdf4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for catching this. working on adding a doc
section, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is nbsphinx
necessary for docs? I think I've had an issue withthout this in the past
setup.py
Outdated
'complete': ['scipy', 'tables', 'numba', 'siphon', 'ephem'], | ||
'complete': ['scipy', 'tables', 'numba', 'siphon', 'netcdf4', 'ephem'], | ||
'doc': ['sphinx', 'ipython', 'sphinx_rtd_theme', 'numpydoc', | ||
'matplotlib', 'jupyter'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe nbsphinx
is also necessary for docs
for reference, here's my |
Your configuration file uses an unknown linter. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since #568 don't need no Sphinx or jupyter anymore. Thanks! Can you paste a link to your rtfd build please?
Your configuration file uses an unknown linter. The |
Here's a link to the build: https://wholmgren-pvlib-python-new.readthedocs.io/en/extras/index.html but none of the changes in this PR should effect the documentation build on readthedocs. I'm wondering if we'd be better off with something like
|
|
Your configuration file uses an unknown linter. The |
Ok @mikofski, now the changes in this PR do effect the doc build! Between wheels and readthedocs.org including numpy/scipy/matplotlib in its root environment, I think we're now better off using pip installs on RTD than conda envs. This PR consolidates the build requirements in one place ( Unfortunately, reconfiguring RTD has not fixed the ipython directive issue. (I think that is an ipython directive issue or an issue with how we're using it rather than an RTD issue.) |
Thanks so much for working so hard on this! |
Well I had issues with it locally so not sure what to ask specific to rtd.
…On Sun, Sep 9, 2018 at 8:00 PM Mark Mikofski ***@***.***> wrote:
At some point does it make sense asking on Google groups
<https://groups.google.com/forum/m/#!forum/read-the-docs> make sense? Or
searching SO <https://stackoverflow.com/questions/tagged/read-the-docs>?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#566 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AELiR0B7HKyYRyVBjcpjyPjBR1cxsbxXks5uZdW7gaJpZM4WeAwk>
.
|
Filed an issue with IPython: ipython/ipython#11298 |
@mikofski are you happy with the latest changes here? You are the active contributor that is most likely to take advantage of this on a regular basis, so want to make sure you're happy with it. I'm ready to merge if you still approve. The latest doc build (still on conda) now shows the same ipython behavior as in the build of my fork's branch. I think we should go ahead with changing the pvlib RTD configuration to use wheels. |
LGTM, if I have a chance, I'll try to figure out what's going on with ipython directive, maybe there's an alterate way to do the doctest, but for now I think this is fine. Thanks again! OK with me to merge ... |
docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
file for all changes.I'm not willing to make scipy, tables, etc. requirements for the upcoming 0.6.0 release. But I think we can use the setuptools extras_require option to at least make it a little easier for a few people. So people will be able to install all of the dependencies necessary for any feature by running
edited below here for clarity and future reference
I originally chose "complete" because that's what dask uses. See discussion below for reasons to changing to optional/doc/test/all.
Options can also be combined like this:
@mikofski you are most qualified to review this!