-
Notifications
You must be signed in to change notification settings - Fork 11
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
Introduction of Dask for all data manipulations #312
Conversation
Note that the new |
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 am satisfied the functionality is all good here, and is preserved and inherited fine downstream in cf-python when using both this branch and the corresponding branch with removal from cf, NCAS-CMS/cf-python#836.
As usual I've raised some minor comments, mostly where docstrings explicitly reference the cf
module which needs updating to {{package}}
(else cfdm
) now.
I was surprised we didn't need any packaging updates, but I double-checked and it seems we already added dask
as a dependency to cfdm so that's sorted, and from running the command python -c "from setuptools import find_packages; from pprint import pprint; pprint(find_packages())"
it looks like, despite many new data modules including a new __init__.py
, there is nothing to add or remove from the setup.py
packages
key, so the setup.py
doesn't need updating, as you likely determined yourself.
Overall all good, so once you've considered the minor comments in-line, feel free to merge. (Note I've been reviewing NCAS-CMS/cf-python#836 in cross-reference but still have some further checks to do on that side, so will submit the review for that shortly but not right away.)
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
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 addressing the (very) minor feedback items. Did a final sanity check and everything's good so feel free to merge (or I'll be doing the final review on the corresponding cf-python PR later today if you want to wait on that to merge them together).
Great - thanks, Sadie! I'll hold off merging until the corresponding |
Following on from our off-line discussion on @classmethod
def _binary_operation(cls, data, other, method):
"""Binary arithmetic and comparison operations.
<snip>
"""
# Note: This method is a classmethod to allow it's
# functionality to be used with a LHS operand that is
# not 'self'. This is not currently needed here, but
# could be useful in subclasses which overload this
# method, yet still want to access the parent method
# functionality via `super`. For instance, cf-python's
# `cf.Data._binary_operation` sometimes needs to apply
# the binary operation to a modified copy of its 'self'.
inplace = method[2] == "i" |
Does to me! Thanks, all good with that. |
Great. I'll add it 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.
Thanks for addressing and discussing my concerns from the cf-python PR. I am happy with the explanatory comment here (one typo). Please merge (both PRs) once you feel ready!
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Merging - thanks for the review, Sadie! |
Fixes #317
Implements Dask for all storage underlying
cfdm.Data
objects, by porting the corresponding code fromcf-python
.Aims to not change the API, other than as required for the new functionality.