Get core Dask functionality from cfdm
#836
Conversation
sadielbartholomew
left a comment
There was a problem hiding this comment.
Looks good overall except for the case of the updated API for the binary operations stemming from a change to _binary_operation (which may have been deliberate but I suspect not) - see my inline comments regarding that which summarise my thoughts.
Otherwise just the usual minor comments. So happy to approve once we've considered/discussed the above issue. Thanks for your patience on this review.
| new_data = field0.data._binary_operation( | ||
| field0.data, field1.data, method | ||
| ) |
There was a problem hiding this comment.
I don't like this API whereby we are specifying field0.data as both the object of the method and an argument - is unnecessary and could attract bugs if the object and the first argument are different. I actually imagine you didn't intend this and missed this detail in the translation, but just to check?
It looks like the issue (assuming it wasn't intended as above) stems from including and using the Data data as an argument to def _binary_operation(cls, data, other, method) in the method translated to cfdm. If we drop the superfluous argument data then it should be fine - but this will mean quite a few updates here and perhaps in the corresponding cfdm PR too, e.g. from a git grep "_binary_oper" in the cf directory I see there are many methods to tweak:
data/data.py: return self._binary_operation(self, other, "__add__")
data/data.py: return self._binary_operation(self, other, "__iadd__")
data/data.py: return self._binary_operation(self, other, "__radd__")
data/data.py: return self._binary_operation(self, other, "__sub__")
data/data.py: return self._binary_operation(self, other, "__isub__")
data/data.py: return self._binary_operation(self, other, "__rsub__")
data/data.py: return self._binary_operation(self, other, "__mul__")
data/data.py: return self._binary_operation(self, other, "__imul__")
data/data.py: return self._binary_operation(self, other, "__rmul__")
data/data.py: return self._binary_operation(self, other, "__div__")
data/data.py: return self._binary_operation(self, other, "__idiv__")
data/data.py: return self._binary_operation(self, other, "__rdiv__")
data/data.py: return self._binary_operation(self, other, "__floordiv__")
data/data.py: return self._binary_operation(self, other, "__ifloordiv__")
data/data.py: return self._binary_operation(self, other, "__rfloordiv__")
data/data.py: return self._binary_operation(self, other, "__truediv__")
data/data.py: return self._binary_operation(self, other, "__itruediv__")
data/data.py: return self._binary_operation(self, other, "__rtruediv__")
data/data.py: return self._binary_operation(self, other, "__pow__")
data/data.py: return self._binary_operation(self, other, "__ipow__")
data/data.py: return self._binary_operation(self, other, "__rpow__")
data/data.py: return self._binary_operation(self, other, "__mod__")
data/data.py: return self._binary_operation(self, other, "__imod__")
data/data.py: return self._binary_operation(self, other, "__rmod__") There was a problem hiding this comment.
There are so many I won't make (GH) 'suggestions' here on the PR, but I'll emphasise this one on the main review comment.
There was a problem hiding this comment.
I'm trying to remember why I thought is was necessary to make _binary_operation a class method. I'll get back to you ...
There was a problem hiding this comment.
No worries - the more reviewing I did the more I changed my mind and thought it might indeed be deliberate, to update my comment above!
| if not inplace: | ||
| new = self.copy() # data=False) TODO | ||
| new_data = data._binary_operation(y, method) | ||
| new_data = data._binary_operation(data, y, method) |
There was a problem hiding this comment.
(Relates to a previous comment) why need this to be specified when it is the object the method operates on?
sadielbartholomew
left a comment
There was a problem hiding this comment.
A few minor comments left to address, some newly-introduced, but then I am happy, so please merge 🙂
Fixes #839
Remove core Dask functionality, importing it from
cfdminstead.Requires NCAS-CMS/cfdm#312 from
cfdm.Aims to not change the API, other than as required by the new
cfdm.