Skip to content
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.isclose #411

Merged
merged 12 commits into from
Jun 21, 2022
Merged

Conversation

davidhassell
Copy link
Collaborator

No description provided.

@davidhassell davidhassell added the dask Relating to the use of Dask label Jun 21, 2022
@davidhassell
Copy link
Collaborator Author

Hi Sadie. I'm having all sort of linting issues, having upgraded black. I get different results when I run pre-commit compared with running black on its own, and it's all different to "before" ...

Anyway, the only interesting parts of this PR are Data.isclose, Data._binary_operation, utils.conform_units, test_Data.py.

Sorry!

@sadielbartholomew
Copy link
Member

Hi Sadie. I'm having all sort of linting issues, having upgraded black. I get different results when I run pre-commit compared with running black on its own, and it's all different to "before" ...

Anyway, the only interesting parts of this PR are Data.isclose, Data._binary_operation, utils.conform_units, test_Data.py.

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.

Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One probable (essentially pre-existing) typo to blitz but otherwise perfect, so please merge when ready. Thanks.

cf/data/data.py Outdated Show resolved Hide resolved
Comment on lines +9199 to +9208
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elegant approach! +1

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to take the credit, but it's approach I pinched from parts of the dask code base :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, might as well include minor things in other PRs to avoid a long chain of PRs :)

Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
@davidhassell davidhassell changed the title Dask isclose dask: Data.isclose Jun 21, 2022
@davidhassell davidhassell merged commit c7ed270 into NCAS-CMS:lama-to-dask Jun 21, 2022
@davidhassell davidhassell deleted the dask-isclose branch November 15, 2022 09:25
@davidhassell davidhassell added this to the 3.14.0 milestone Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dask Relating to the use of Dask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants