issue_comments

@shoyer, I added the notes

@max-sixty, @jthielen: the identical tests will be skipped for now. At the moment that does not make any difference since identical is the same as equals with additionally checking the attributes (any changes to it are not limited to pint, so I guess we should open a new issue for discussion).

If strict unit checking is required, I think that may be better served by an additional assert unit == "meter" type statement.

which is what I've been doing with assert_units_equal. I'll change the tests for identical, then.

Also, concerning the commutative operations: should we wait for hgrecco/pint#1019 and remove the flipped parameters or should we merge as is and possibly revert once pint implements a type casting hierarchy?

when writing the unit tests, we decided the definition of equals to be different from identical (see https://github.com/pydata/xarray/pull/3238#issuecomment-533145925 and https://github.com/pydata/xarray/pull/3238#issuecomment-533181017) which would be consistent with the behaviour of identical when a "units" attribute is set.

As far as I know there was no real use case (except from being able to use assert_identical in the unit tests), so we can change that.

cc @jthielen

it should be ready now

Edit: the failures are unrelated (#3751) Edit2: the documentation of this change does not exist, yet. It will be added in a new PR and is a task in #3594

this should be ready for review and merge once the tests pass.

these issues (rolling_window and identical) seem too big to discuss here, so in order to keep moving forward let's mark these tests as xfail and discuss this in their own issues / PRs.

gentle ping, @shoyer

I think a separate units_equiv function may be cleaner?

I agree. However, I'm reluctant to add that function since that would be the first pint dependent code we have except from the unit tests (but do tell me if that's not an issue).

I'm inclined to provide some kind of hook for wrapped libraries instead (allow them to define a method like metadata_identical or something that does the check for us) so the code in identical would become something like ```python def metadata_identical(arr1, arr2): if hasattr(arr1, "metadata_identical"): return arr1.metadata_identical(arr2) elif hasattr(arr2, "metadata_identical"): return arr2.metadata_identical(arr1) else: return True

return ( self.dims == other.dims and (self._data is other._data or equiv(self.data, other.data)) and metadata_identical(self.data, other.data) ) ```

Note that we explicitly use lazy_array_equiv in concat so it'd be nice to have something that could be easily used there too

I don't think that's a problem since what calls lazy_array_equiv is Variable.identical and if identical works correctly this should, too.

hrmm... I had investigated this before and thought I remembered correctly. I can't put it in lazy_array_equiv, though, since array_equiv is also used by equals (which should not check the units, only the dimensionality)

I think your units_equiv thing could go in duck_array_ops.lazy_array_equiv

that could work for Dataset.identical but not Variable.identical, which I believe by default directly calls array_equiv

re rolling_window: should I leave it as xfail for now?

the easiest way to fix identical would be to pass a equiv function that forwards to duck_array_ops.array_equiv but also checks the units: ```python def units_equiv(a, b): if hasattr(a, "units") or hasattr(b, "units"): units_a = getattr(a, "units", None) units_b = getattr(b, "units", None)

registry = getattr(units_a, "_REGISTRY", None) or getattr(b, "_REGISTRY", None)

units_a = units_a or registry.dimensionless
units_b = units_b or registry.dimensionless

return units_a == units_b
else:
# no units at all
return True

def equiv_with_units(a, b): return duck_array_ops.array_equiv(a, b) and units_equiv(a, b) `` However, I'm out of ideas on how to automatically use that (I'd like to avoid having to calla.identical(b, equiv=equiv_with_units)`).

There is also rolling_window that strips the units due to calling numpy.lib.stride_tricks.as_strided, but I don't know how to fix that unless we rewrite _rolling_window or dispatch differently in rolling_window.

There seems to have been some work to add a function named numpy.rolling_window or numpy.sliding_window that could then be overridden by pint, but I think that effort has stalled?

I'm not sure why, but dask==1.2 fails where later versions work: pytb ValueError: operands could not be broadcast together with shapes (2,0) (2,2)

Edit: Seems we can just bump dask to dask==2.1 or dask==2.2 and not worry about this. I'd wait on #3660 with this.

I fixed the tests that were easier, but I have no idea about rolling_window / as_strided because I don't understand how it works, and identical would be easy to fix if we special-cased pint, but I'd like to avoid that

Edit: seems there are some issues with np.pad support in dask==1.2: __array_function__ support is probably only in later versions so we'd need to use da.pad instead, but that raises an error, too.