Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/whats-new.rst
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ Bug fixes
now reopens the file from scratch for h5netcdf and scipy netCDF backends,
rather than reusing a cached version (:issue:`4240`, :issue:`4862`).
By `Stephan Hoyer <https://github.com/shoyer>`_.
- Fix handling of coordinate attributes in ``xarray.where``. (:issue:`7220`, :pull:`7229`)
By `Sam Levang <https://github.com/slevang>`_.

Documentation
~~~~~~~~~~~~~
Expand Down
2 changes: 1 addition & 1 deletion xarray/core/computation.py
Original file line number Diff line number Diff line change
Expand Up @@ -1860,7 +1860,7 @@ def where(cond, x, y, keep_attrs=None):
if keep_attrs is True:
# keep the attributes of x, the second parameter, by default to
# be consistent with the `where` method of `DataArray` and `Dataset`
keep_attrs = lambda attrs, context: getattr(x, "attrs", {})
keep_attrs = lambda attrs, context: attrs[1] if len(attrs) > 1 else {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we factor this out to a helper as suggested by @keewis?


# alignment for three arguments is complicated, so don't support it yet
return apply_ufunc(
Expand Down
32 changes: 26 additions & 6 deletions xarray/tests/test_computation.py
Original file line number Diff line number Diff line change
Expand Up @@ -1923,16 +1923,36 @@ def test_where() -> None:


def test_where_attrs() -> None:
cond = xr.DataArray([True, False], dims="x", attrs={"attr": "cond"})
x = xr.DataArray([1, 1], dims="x", attrs={"attr": "x"})
y = xr.DataArray([0, 0], dims="x", attrs={"attr": "y"})
cond = xr.DataArray([True, False], coords={"x": [0, 1]}, attrs={"attr": "cond_da"})
cond["x"].attrs = {"attr": "cond_coord"}
x = xr.DataArray([1, 1], coords={"x": [0, 1]}, attrs={"attr": "x_da"})
x["x"].attrs = {"attr": "x_coord"}
y = xr.DataArray([0, 0], coords={"x": [0, 1]}, attrs={"attr": "y_da"})
y["x"].attrs = {"attr": "y_coord"}

# 3 DataArrays, takes attrs from x
actual = xr.where(cond, x, y, keep_attrs=True)
expected = xr.DataArray([1, 0], dims="x", attrs={"attr": "x"})
expected = xr.DataArray([1, 0], coords={"x": [0, 1]}, attrs={"attr": "x_da"})
expected["x"].attrs = {"attr": "x_coord"}
assert_identical(expected, actual)

# ensure keep_attrs can handle scalar values
# x as a scalar, takes attrs from y
actual = xr.where(cond, 0, y, keep_attrs=True)
expected = xr.DataArray([0, 0], coords={"x": [0, 1]}, attrs={"attr": "y_da"})
expected["x"].attrs = {"attr": "y_coord"}
assert_identical(expected, actual)

# y as a scalar, takes attrs from x
actual = xr.where(cond, x, 0, keep_attrs=True)
expected = xr.DataArray([1, 0], coords={"x": [0, 1]}, attrs={"attr": "x_da"})
expected["x"].attrs = {"attr": "x_coord"}
assert_identical(expected, actual)

# x and y as a scalar, takes coord attrs only from cond
actual = xr.where(cond, 1, 0, keep_attrs=True)
assert actual.attrs == {}
expected = xr.DataArray([1, 0], coords={"x": [0, 1]})
expected["x"].attrs = {"attr": "cond_coord"}
assert_identical(expected, actual)
Copy link
Contributor Author

@slevang slevang Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives basically the same result as prior to #6461, where the doc statement that we only take attrs from x is misleading. I actually like the way it works now, and found it hard to implement something that only pulled from x and didn't break other uses of apply_variable_ufunc. If we're ok maintaining this behavior, I tweaked the docstring slightly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems confusing but I don't have a strong opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it doesn't take the cond coord attrs. I still think this would be a convenient (albeit confusing) feature, because I happen to have a bunch of code like xr.where(x>10, 10, x) and would rather keep the attrs. But this is obviously a better use case for DataArray.where.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.



@pytest.mark.parametrize(
Expand Down