Skip to content

Don't drop unreduced variables #5393

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.

Sign up for GitHub

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

Merged
max-sixty merged 4 commits into from
Jun 12, 2021
Merged

Don't drop unreduced variables #5393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eae9990
don't drop unreduced variables
malmans2 May 28, 2021
47dece1
Update xarray/tests/test_dataset.py
malmans2 May 31, 2021
6911fb7
Merge branch 'pydata:master' into fix_reduce
malmans2 Jun 11, 2021
da25df9
add comment
malmans2 Jun 11, 2021
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
5 changes: 4 additions & 1 deletion xarray/core/dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4987,7 +4987,10 @@ def reduce(
variables[name] = var
else:
if (
not numeric_only
# Some reduction functions (e.g. std, var) need to run on variables
# that don't have the reduce dims: PR5393
not reduce_dims

@keewis keewis May 31, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

actually, wouldn't it make more sense to add unreduced variables without calling reduce? Something like

if not reduce_dims:
    variables[name] = var
elif not numeric_only or np.issubdtype(var.dtype, nb.number) or var.dtype == np.bool_:
    # ...
    variables[name] = var.reduce(...)

@malmans2 malmans2 May 31, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That was my first idea, but a few test failed, so it would be a breaking change. I think it mainly has to do with how attributes are handled by reduce.

@keewis keewis Jun 10, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

well, attrs seems to be one of the issues (but one we could fix!). However, some reduction functions (e.g. std, var) need to run on variables that don't have the reduce dims, so my suggestion is actually wrong. See also the concept of invariant_0d introduced in #5207.

As I'm sure I will have forgotten about this in a few months: could you add a comment explaining this?

@malmans2 malmans2 Jun 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@keewis keewis Jun 11, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great, thanks!

@keewis keewis Jun 11, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

last question: if I'm understanding this correctly, not numeric_only and all following lines will be overridden by not reduce_dims. Should we move that three lines down, below the dtype checks?

or not numeric_only
or np.issubdtype(var.dtype, np.number)
or (var.dtype == np.bool_)
):
Expand Down
5 changes: 3 additions & 2 deletions xarray/tests/test_dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4947,15 +4947,16 @@ def test_reduce_cumsum_test_dims(self, reduct, expected, func):
def test_reduce_non_numeric(self):
data1 = create_test_data(seed=44)
data2 = create_test_data(seed=44)
add_vars = {"var4": ["dim1", "dim2"]}
add_vars = {"var4": ["dim1", "dim2"], "var5": ["dim1"]}
for v, dims in sorted(add_vars.items()):
size = tuple(data1.dims[d] for d in dims)
data = np.random.randint(0, 100, size=size).astype(np.str_)
data1[v] = (dims, data, {"foo": "variable"})

assert "var4" not in data1.mean()
assert "var4" not in data1.mean() and "var5" not in data1.mean()
assert_equal(data1.mean(), data2.mean())
assert_equal(data1.mean(dim="dim1"), data2.mean(dim="dim1"))
assert "var4" not in data1.mean(dim="dim2") and "var5" in data1.mean(dim="dim2")

@pytest.mark.filterwarnings(
"ignore:Once the behaviour of DataArray:DeprecationWarning"
Expand Down