-
Notifications
You must be signed in to change notification settings - Fork 19
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
dask: Data.percentile
, Data.median
#313
dask: Data.percentile
, Data.median
#313
Conversation
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.
This is looking good generally, but I have some questions and comments...
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
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.
In re-review I noticed some typos in docstrings and comments that could ideally be fixed, as raised in-line, but overall this appears to be watertight, with adapted and improved testing, and all feedback raised previously has been addressed well. Nice! Feel free to merge.
Actually, just thinking we should the check the CI jobs pre-merge, triggering those via open-close... |
The 3.7 job has failed on environment setup because of the new
|
Hi Sadie, I think that the choice is between disallowing the latest numpy, or disallowing the oldest Python. I favour the latter. If people really won't/can't drop python 3.7 then they can still use cf-python<v4.0.0 and numpy<1.22. Also the latest dask is imminently going to drop Python 3.7: dask/community#213 |
... so I think removing 3.7 from the CI tests would be fine! |
Agreed, with that justification 🙂 |
(Agreed in a Slack call that I will update the CI workflows to not run on 3.7 in a follow-on PR given we are dropping it as discussed briefly above.) |
Merging, and I will get the workflows updated RE Python 3.7. Thanks again David. |
No description provided.