issue_comments

Nice, that seems to work!

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

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.

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! :beers:

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

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?

The 3.7 test looks like an installation error.

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

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

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

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.

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.

@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): https://github.com/pydata/xarray/blob/6d55f99905d664ef73cb708cfe8c52c2c651e8dc/xarray/core/dataarray.py#L193

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.