dask: Data.isclose#411
Conversation
|
Hi Sadie. I'm having all sort of linting issues, having upgraded Anyway, the only interesting parts of this PR are Sorry! |
Hi David, no worries, since I can upgrade the linting workflows and such in line with this, but sorry if it was a pain with regards to this PR. I'll take a look later today. |
sadielbartholomew
left a comment
sadielbartholomew
left a comment
There was a problem hiding this comment.
One probable (essentially pre-existing) typo to blitz but otherwise perfect, so please merge when ready. Thanks.
| a = np.empty((), dtype=self.dtype) | ||
| b = np.empty((), dtype=da.asanyarray(y).dtype) | ||
| try: | ||
| # Check if a numerical isclose is possible | ||
| np.isclose(a, b) | ||
| except TypeError: | ||
| # self and y do not have suitable numeric data types | ||
| # (e.g. both are strings) | ||
| return self == y | ||
| else: |
There was a problem hiding this comment.
I'd like to take the credit, but it's approach I pinched from parts of the dask code base :)
There was a problem hiding this comment.
I prefer the phrase 'was inspired by' over 'pinched' or similar!
| # ------------------------------------------------------------ | ||
| if method == "__eq__": | ||
| result = da.isclose(dx0, dx1, rtol=rtol, atol=atol) | ||
| if dx0.dtype.kind in "US" or dx1.dtype.kind in "US": |
There was a problem hiding this comment.
Good spot and catch. Maybe we should update the test that covers the various binary operations, test_Data_BINARY_AND_UNARY_OPERATORS, to check some/more string type operations (not necessarily in this PR)? (I am already updating it a little to do some more complex unit checking to cover _combined_units (or raising an issue to register that this should be done at some point) as we discussed recently. Maybe I can do it as part of that update.)
There was a problem hiding this comment.
Doing as part of that update sounds good. I wavered about putting the fix in to _binary_operator in this PR, but decided to go for it as it was convenient, and might have been the only update to this area.
There was a problem hiding this comment.
All good, might as well include minor things in other PRs to avoid a long chain of PRs :)
2 participants
No description provided.