Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix coordinate attr handling in xr.where(..., keep_attrs=True) #7229

Merged
merged 17 commits into from
Nov 30, 2022
Merged
2 changes: 2 additions & 0 deletions doc/whats-new.rst
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,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>`_.
- Fixed bug where :py:meth:`Dataset.coarsen.construct` would demote non-dimension coordinates to variables. (:pull:`7233`)
By `Tom Nicholas <https://github.com/TomNicholas>`_.
- Raise a TypeError when trying to plot empty data (:issue:`7156`, :pull:`7228`).
Expand Down
5 changes: 3 additions & 2 deletions xarray/core/computation.py
Original file line number Diff line number Diff line change
Expand Up @@ -1789,7 +1789,8 @@ def where(cond, x, y, keep_attrs=None):
y : scalar, array, Variable, DataArray or Dataset
values to choose from where `cond` is False
keep_attrs : bool or str or callable, optional
How to treat attrs. If True, keep the attrs of `x`.
How to treat attrs. If True, keep the attrs of `x`,
unless `x` is a scalar, then keep the attrs of `y`.

Returns
-------
Expand Down Expand Up @@ -1860,7 +1861,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 @@ -1925,16 +1925,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