dask: `Data.isclose` by davidhassell · Pull Request #411 · NCAS-CMS/cf-python

dask: `Data.isclose` by davidhassell · Pull Request #411 · NCAS-CMS/cf-python

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

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.

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:

Choose a reason for hiding this comment

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

Elegant approach! +1

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

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":

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

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.

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>