Check shapes of coordinates and data during DataArray construction

[lilyminium]

• Closes numpy.insert on DataArray may silently result in array inconsistent with its coordinates  #2529 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

This sets DataArrayGroupBy.reduce(shortcut=False), as the shortcut first constructs a DataArray with the previous coordinates and the new mutated data before updating the coordinates; this order of events now raises a ValueError.

[pep8speaks]

Hello @lilyminium! 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! 🍻

Comment last updated at 2021-05-31 21:16:46 UTC

[shoyer]

@lilyminium thanks for looking into this! I think the specific problem here is due to our use of the shortcut _replace method in DataArray.__array_wrap__ (which is called by NumPy, e.g., inside np.insert):

xarray/xarray/core/dataarray.py

Line 193 in 6d55f99

encoding : dict_like or None, optional

I think the clean way to fix this would be to add error checking to DataArray.__array_wrap__. But we currently use this method inside DataArray._unary_op (and maybe elsewhere?), which is called every time you do unwary math with an xarray object (e.g., - ). We’ll want to update these uses to use a method that uses the same shortcut to avoid redundant metadata checks.

[shoyer]

This could be pretty expensive — it means we have to check shape consistency every time we create a DataArray, even if we know that the shape is already consistent, e.g., because it was created by another xarray operation.

[lilyminium]

I think the clean way to fix this would be to add error checking to DataArray.__array_wrap__. But we currently use this method inside DataArray._unary_op (and maybe elsewhere?), which is called every time you do unwary math with an xarray object (e.g., - ). We’ll want to update these uses to use a method that uses the same shortcut to avoid redundant metadata checks.

Does adding fastpath to skip shape checking in array_wrap work? fastpath=True in both DataArray.array_wrap and Variable.array_wrap didn't seem to break any tests.

[shoyer]

Does adding fastpath to skip shape checking in array_wrap work? fastpath=True in both DataArray.array_wrap and Variable.array_wrap didn't seem to break any tests.

I think it would be slightly better to stick with a separate method for xarray's fast-path. __array_wrap__ is a special protocol used by NumPy, so there's always a (unlikely) risk that if we use different arguments in our version it could break in the future.

[max-sixty]

This would still be valuable if @lilyminium (or someone else, if they're not interested) would want to finish this off.

[lilyminium]

Ooh, this is a blast from the past. Anyone passing by, feel free to pick this up if you like, otherwise I'll see what I can do over the weekend :)

[lilyminium]

Hmmmm. It's been quite some time since I've last used xarray, but I'm not sure I see an easy way to:

1. hijack np.insert (which I would guess goes directly to DataArray.__array_wrap__ / __array_finalize__)
2. without passing a fastpath-like argument to __array_wrap__, but
3. efficiently not checking the shape of safe operations, like _unary_op.

I have for now stuck fastpath in _replace but it fails condition 3 and just runs for all calls to __array_wrap__.

[lilyminium]

The 3.7 test looks like an installation error.

[max-sixty]

Hmmmm. It's been quite some time since I've last used xarray, but I'm not sure I see an easy way to:

1. hijack np.insert (which I would guess goes directly to `DataArray.__array_wrap__ / __array_finalize__`)

2. without passing a `fastpath`-like argument to `__array_wrap__`, but

3. efficiently not checking the shape of safe operations, like `_unary_op`.

I think that's exactly correct. Without being able to tell __array_wrap__ from _unary_op that we don't need to check the sizes, they all follow the same path. Is it possible to use the context arg for this? I don't think it's sufficiently flexible from an initial glance of the docs.

Maybe this new implementation has a low enough overhead that it's worthwhile?

If we do go ahead — what about calling _check_shape_consistency from within __array_wrap__ after creating the object, instead of adding the fastpath kwarg?

[lilyminium]

context

I can only find references regarding ufuncs in the numpy docs when I look up the context. I would think that in that case the shapes should be fine, but please do correct me if not?

_unary_op calls __array_wrap__(context=None); if the function only checks the shape when context is None and a dummy value gets passed in for the context, that's one way to avoid it.

[max-sixty]

Nice, that seems to work!

Is anyone else more familiar with whether using context in this way is reasonable?