issue_comments

Thanks @keewis

I believe it is best to allow customizing the behavior, so I added a keep_attrs parameter. I named the parameter keep_attrs but it could just as well be called combine_attrs (not sure which is better?) It defaults to dropping the attrs (keep_attrs=False) and will keep the attrs of x for keep_attrs=True. Because we're using apply_ufunc under the hood any value accepted by merge_attrs can be passed, with the only difference being that keep_attrs=True is not equal to keep_attrs="override".

Edit: using a different keep_attrs handler does not affect the name of the resulting object. Because this potentially seems like a breaking change I would do that change in a different PR if we think it is important.

Shall we merge this? The doctest seems easy to fix. I was just a little surprised that name was considered an attrs as well, but I might not have looked closely enough on that before.

yes, that's the plan (we changed the keep_attrs to affect both data / data variables and coordinates in a earlier release, although that's not consistent yet), but I'm surprised to hear that xr.where didn't drop attrs in 0.17.

I'm a bit busy right now, but once things calm down a bit I'll try to finish this.

Hi, I have a question about this fix relating to coordinate attributes (and am not sure if that deserves a separate issue): In xarray version 0.17.0, coordinate attributes are being preserved when using xr.where. But, in version 0.19.0, that is not the case anymore. Does the solution for preserving DataArray attributes as discussed here also solve (or rather re-enable) preservation of coordinate attributes? It looks like DataArray.where still preserves coordinate attributes even in version 0.19.0, so I would hope the answer is yes?

@keewis presumbly this could be merged in for the v0.19 release now? #5588

I'm hoping to wait until combine_attrs is in apply_ufunc, so this is blocked by #5041

Shall we merge?

Is that because apply_ufunc(..., keep_attrs=True) keeps the attrs of the first argument?

yes, exactly. However, I think it would be easier to extend keep_attrs as described in https://github.com/pydata/xarray/issues/3891#issuecomment-773693380 and allow passing keep_attrs to xr.where.

I can get there by replacing duck_array_ops.where with duck_array_ops.where_method

Is that because apply_ufunc(..., keep_attrs=True) keeps the attrs of the first argument? Could you restore the dimension order after the apply_ufunc or is it not possible to figure it out? Then I'd be fine with a changed order...

I would think the closest match is to return from x — that's the equivalent of x.where(cond, other=y?

I'm not sure how to implement that: I can get there by replacing duck_array_ops.where with duck_array_ops.where_method, but that yields a different dimension order after broadcasting (for the doctest examples ["y", "x"] instead of ["x", "y"]). This might not be that much of an issue since we generally tell people to use transpose before applying a function that depends on the order, though.

I would think the closest match is to return from x — that's the equivalent of x.where(cond, other=y)?

Great idea!

I would think the closest match is to return from x — that's the equivalent of x.where(cond, other=y?

yes, that's true. Maybe that's why we did not propagate the attrs before, even though it is really easy to implement?

I guess name should be None since we potentially have three possibilities:cond, x, and y?

But that reasoning would also say we should not propagate attrs haha. Not sure what to do.

After thinking about this some more, I decided that it would be good to have the behavior of where mirror the behavior of DataArray.where, Dataset.where, and Variable.where. Should we want to be able to control this using set_options we should do so for all them at once.

I also added a entry to whats-new.rst, but I'm not sure about putting this into Breaking changes. Is Bug fixes more appropriate?

Edit: it seems this also preserves the name of cond, which might not be what we want.