issue_comments

@dcherian No problem! Thanks for all your help!

Things seem to have died down here so let's test it out.

Thanks @rpmanser This is an amazing first PR. Thanks for your efforts and welcome to xarray!

LGTM. The reason for requires_pint_0_15 would be nice but I don't feel strongly about it so it's up to you.

Thanks for being patient, @rpmanser.

No worries, thanks for the reviews!

Hello @rpmanser! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-08-31 21:28:01 UTC

you mean the commit message? If so, you can use git commit --amend to change it (make sure you don't have anything staged because that would be added to the commit) and force-push. It's not that important, though, I doubt anyone really reads the commit messages.

I meant is_duck_array_or_ndarray, not is_duck_dask_array_or_ndarray. I don't suppose there's a way to fix that at this point?

we went with https://github.com/pydata/xarray/blob/edd5c1e96204d59d30496e3f85e2f805a911ac9b/xarray/core/variable.py#L938 in #4379, you could use something like that here: https://github.com/pydata/xarray/blob/3337b6fc4024dcaa2c11831e6d34551adaee4a8b/xarray/core/formatting.py#L554 too, e.g. with: python def is_duck_array_or_ndarray(array): return is_duck_array(array) or (not IS_NEP18_ACTIVE and isinstance(data, np.ndarray))

yes, the failing test is definitely due to NEP18: there's a numpy array in an attribute, but since numpy 1.15 doesn't define __array_function__, is_duck_array returns False. We could extend is_duck_array with an isinstance check for numpy.ndarray, or we could take your suggestion and xfail the test. Not sure which is better.

I would suggest adding an explicit check for NumPy arrays. Otherwise we would need to bump our minimum supported NumPy version.

yes, the failing test is definitely due to NEP18: there's a numpy array in an attribute, but since numpy 1.15 doesn't define __array_function__, is_duck_array returns False. We could extend is_duck_array with an isinstance check for numpy.ndarray, or we could take your suggestion and xfail the test. Not sure which is better.

black recently released a new version, which changed the way it treats a trailing comma, and it also reformats docstrings (at least as much as it can without breaking formats), but neither conda nor the pre-commit hook install the new version (yet). To make the diff of this PR as small as possible, I'm opening a new PR with just the automatic changes, once that is merged, it should be fixed by merging in master.

Edit: see #4381

Tests are failing in test_formatting.py for test_diff_attrs_repr_with_array. This looks very similar to the previous problem that was occurring for duck array equality due to NEP18. Is a similar xfail appropriate here?

I'm not sure why FormattingBlack failed. It's claiming that 45 files would be reformatted. I used the pre-commit hook, which passed.

I have erred on the side of leaving the isinstance checks in place where @jthielen reviewed them. If it is decided that no further tests are needed, then this should be ready for review.

On another note, if it is appropriate to apply these changes:

Revisiting it I now realize that is_array_like is actually not a good name for that function: officially, "array_like" means that it can be casted using np.asarray. Maybe we should rename that to is_duck_array? But then we'd also need to check for __array_ufunc__ and __array_function__ because we also need those to properly wrap duck arrays.

As long as that doesn't break any of the current uses, I think that would be the best way forwards. This would require xarray to be on NumPy 1.16+ (in order to ensure hasattr(np.ndarray, "__array_function__"), but that's only 9 days away by NEP 29, so hopefully that isn't too much of an issue. Then, most of the checks for "can this be wrapped by xarray" should be able to become something like

python is_duck_array(x) and not isinstance(x, (DataArray, Variable))

with is_duck_dask_array only being needed for checking Dask Array-like support.

I can do that as well. If not, should a new issue be opened for this?

Thanks for working on this @rpmanser.

Since you've changed every isinstance check, if test_dask passes, you're doing fine in general.

Test that a duck dask Array has attributes of a dask Array and redirects the appropriate methods to those defined by Variable, DataArray, and Dataset?

Seems like this is covered by test_dask already but I could be wrong.

We could add specific pint-as-dask tests to test_units with appropriate xfails (they should run on the upstream-dev test config).

This could be a discussion topic for our next dev meeting on wednesday.

@rpmanser For what it's worth, I'd think doing tests like test_dask.py but with a post-https://github.com/hgrecco/pint/pull/1129 Pint Quantity or suitably mocked wrapper class between xarray and Dask would be best. But, I think this is more of a maintainer-level question.

I'm getting a bit lost within the testing suite while trying to figure out the implications of this change and what kinds of tests are necessary for it. Would a test module for this - Individually test all functions that include the newly implemented is_duck_dask_array()? - Test that a duck dask Array has attributes of a dask Array and redirects the appropriate methods to those defined by Variable, DataArray, and Dataset? - Be similar to test_dask.py? - All, some, or none of the above?

Any suggestions would be greatly appreciated.

Revisiting it I now realize that is_array_like is actually not a good name for that function: officially, "array_like" means that it can be casted using np.asarray. Maybe we should rename that to is_duck_array? But then we'd also need to check for __array_ufunc__ and __array_function__ because we also need those to properly wrap duck arrays.

As long as that doesn't break any of the current uses, I think that would be the best way forwards. This would require xarray to be on NumPy 1.16+ (in order to ensure hasattr(np.ndarray, "__array_function__"), but that's only 9 days away by NEP 29, so hopefully that isn't too much of an issue. Then, most of the checks for "can this be wrapped by xarray" should be able to become something like

python is_duck_array(x) and not isinstance(x, (DataArray, Variable)) with is_duck_dask_array only being needed for checking Dask Array-like support.

Although, while we're at it, do we also need more careful handling of upcast types? I'm not sure if there even are any out there right now (not sure if a HoloViews Dataset counts here), but that doesn't necessarily mean there never will be any.

Revisiting it I now realize that is_array_like is actually not a good name for that function: officially, "array_like" means that it can be casted using np.asarray. Maybe we should rename that to is_duck_array? But then we'd also need to check for __array_ufunc__ and __array_function__ because we also need those to properly wrap duck arrays.

Also, a broader discussion that's I've seen hinted to in the past, but is brought to the forefront by this PR: how should xarray be checking for general duck array types it can wrap?

Right now, it looks like there is a mix of

python is_array_like(data) and python hasattr(data, "__array_function__") or isinstance(data, dask_array_type)

and it would be nice to bring consistency. However, both these checks seem like they'd let too many types through. For example, is_array_like as currently implemented would still let through too much, such as MemoryCachedArray and xarray Variables/DataArrays, and __array_function__ doesn't truly indicate a NumPy duck array on its own (and it really only works to only capture downcast types right now since xarray does not yet implement __array_function__ itself, #3917).