-
Notifications
You must be signed in to change notification settings - Fork 54
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 requirements to require scikit-learn and purge old requirements #204
Update requirements to require scikit-learn and purge old requirements #204
Conversation
Codecov ReportBase: 41.01% // Head: 41.01% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.5.0 #204 +/- ##
==========================================
Coverage 41.01% 41.01%
==========================================
Files 14 14
Lines 2321 2321
==========================================
Hits 952 952
Misses 1369 1369
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Note that the requirements in |
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.
Good job @freemansw1, makes sense to me. There is a minor issue that I have found.
The README.md still references the old name of requirements.txt:
conda install -c conda-forge --yes --file conda-requirements.txt
Good catch! I've updated this. |
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.
Great! Approving this now
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.
Great update @freemansw1!
All right, approved! And sorry that I missed your very clear comment on cartopy and iris! You are probably right that we can not do much about that. |
No worries! It is a bit odd, and I do worry that this will cause us headaches when putting things onto PyPI (#192), but it's the solution that we have for now. |
I had to resolve a merge conflict with #191 merged in. @JuliaKukulies or @snilsn can you give this a quick look-over again and then I will merge? |
Looks good and ready to be merged @freemansw1! |
Thanks! |
Resolves #119 and #196.
Specific actions
pytables
andcf-units
from our dependencies (both are used by our dependencies, but not by us directly)conda-requirements.txt
torequirements.txt
as that seems to be the standard (I cannot find reference toconda-requirements.txt
anywhere)setup.py
such thatpip install .
installs dependencies as well.Standard questions