From 7f20d4f8829f35765efd1385e192befdbf6bf08f Mon Sep 17 00:00:00 2001 From: dcherian Date: Tue, 15 Oct 2019 16:01:48 -0600 Subject: [PATCH 1/6] Another groupby.reduce bugfix. Fixes #3402 --- xarray/core/groupby.py | 28 ++++++++++------- xarray/tests/test_dataarray.py | 9 ------ xarray/tests/test_groupby.py | 56 +++++++++++++++++++++++++--------- 3 files changed, 58 insertions(+), 35 deletions(-) diff --git a/xarray/core/groupby.py b/xarray/core/groupby.py index c52b0dc97e6..347f9817e77 100644 --- a/xarray/core/groupby.py +++ b/xarray/core/groupby.py @@ -22,6 +22,20 @@ from .variable import IndexVariable, Variable, as_variable +def check_reduce_dims(reduce_dims, dimensions): + + if isinstance(reduce_dims, str): + reduce_dims = [reduce_dims] + + if reduce_dims is not ALL_DIMS and any( + [dim not in dimensions for dim in reduce_dims] + ): + raise ValueError( + "cannot reduce over dimensions %r. expected either xarray.ALL_DIMS to reduce over all dimensions or one or more of %r." + % (reduce_dims, dimensions) + ) + + def unique_value_groups(ar, sort=True): """Group an array by its unique values. @@ -794,15 +808,11 @@ def reduce( if keep_attrs is None: keep_attrs = _get_keep_attrs(default=False) - if dim is not ALL_DIMS and dim not in self.dims: - raise ValueError( - "cannot reduce over dimension %r. expected either xarray.ALL_DIMS to reduce over all dimensions or one or more of %r." - % (dim, self.dims) - ) - def reduce_array(ar): return ar.reduce(func, dim, axis, keep_attrs=keep_attrs, **kwargs) + check_reduce_dims(dim, self.dims) + return self.apply(reduce_array, shortcut=shortcut) @@ -895,11 +905,7 @@ def reduce(self, func, dim=None, keep_attrs=None, **kwargs): def reduce_dataset(ds): return ds.reduce(func, dim, keep_attrs, **kwargs) - if dim is not ALL_DIMS and dim not in self.dims: - raise ValueError( - "cannot reduce over dimension %r. expected either xarray.ALL_DIMS to reduce over all dimensions or one or more of %r." - % (dim, self.dims) - ) + check_reduce_dims(dim, self.dims) return self.apply(reduce_dataset) diff --git a/xarray/tests/test_dataarray.py b/xarray/tests/test_dataarray.py index 1398e936f37..301ad27b243 100644 --- a/xarray/tests/test_dataarray.py +++ b/xarray/tests/test_dataarray.py @@ -2560,15 +2560,6 @@ def change_metadata(x): expected = change_metadata(expected) assert_equal(expected, actual) - def test_groupby_reduce_dimension_error(self): - array = self.make_groupby_example_array() - grouped = array.groupby("y") - with raises_regex(ValueError, "cannot reduce over dimension 'y'"): - grouped.mean() - - grouped = array.groupby("y", squeeze=False) - assert_identical(array, grouped.mean()) - def test_groupby_math(self): array = self.make_groupby_example_array() for squeeze in [True, False]: diff --git a/xarray/tests/test_groupby.py b/xarray/tests/test_groupby.py index 957d4800c40..dcd36c8fa3b 100644 --- a/xarray/tests/test_groupby.py +++ b/xarray/tests/test_groupby.py @@ -5,7 +5,23 @@ import xarray as xr from xarray.core.groupby import _consolidate_slices -from . import assert_identical, raises_regex +from . import assert_allclose, assert_identical, raises_regex + + +@pytest.fixture +def dataset(): + ds = xr.Dataset( + {"foo": (("x", "y", "z"), np.random.randn(3, 4, 2))}, + {"x": ["a", "b", "c"], "y": [1, 2, 3, 4], "z": [1, 2]}, + ) + ds["boo"] = (("z", "y"), [["f", "g", "h", "j"]] * 2) + + return ds + + +@pytest.fixture +def array(dataset): + return dataset["foo"] def test_consolidate_slices(): @@ -21,25 +37,17 @@ def test_consolidate_slices(): _consolidate_slices([slice(3), 4]) -def test_groupby_dims_property(): - ds = xr.Dataset( - {"foo": (("x", "y", "z"), np.random.randn(3, 4, 2))}, - {"x": ["a", "bcd", "c"], "y": [1, 2, 3, 4], "z": [1, 2]}, - ) +def test_groupby_dims_property(dataset): + assert dataset.groupby("x").dims == dataset.isel(x=1).dims + assert dataset.groupby("y").dims == dataset.isel(y=1).dims - assert ds.groupby("x").dims == ds.isel(x=1).dims - assert ds.groupby("y").dims == ds.isel(y=1).dims - - stacked = ds.stack({"xy": ("x", "y")}) + stacked = dataset.stack({"xy": ("x", "y")}) assert stacked.groupby("xy").dims == stacked.isel(xy=0).dims -def test_multi_index_groupby_apply(): +def test_multi_index_groupby_apply(dataset): # regression test for GH873 - ds = xr.Dataset( - {"foo": (("x", "y"), np.random.randn(3, 4))}, - {"x": ["a", "b", "c"], "y": [1, 2, 3, 4]}, - ) + ds = dataset.isel(z=1, drop=True)[["foo"]] doubled = 2 * ds group_doubled = ( ds.stack(space=["x", "y"]) @@ -276,4 +284,22 @@ def test_groupby_grouping_errors(): dataset.to_array().groupby(dataset.foo * np.nan) +def test_groupby_reduce_dimension_error(array): + grouped = array.groupby("y") + with raises_regex(ValueError, "cannot reduce over dimensions"): + grouped.mean() + + with raises_regex(ValueError, "cannot reduce over dimensions"): + grouped.mean("huh") + + with raises_regex(ValueError, "cannot reduce over dimensions"): + grouped.mean(("x", "y", "asd")) + + grouped = array.groupby("y", squeeze=False) + assert_identical(array, grouped.mean()) + + assert_identical(array.mean("x"), grouped.reduce(np.mean, "x")) + assert_allclose(array.mean(["x", "z"]), grouped.reduce(np.mean, ["x", "z"])) + + # TODO: move other groupby tests from test_dataset and test_dataarray over here From b1660ecb06268da43fd7cc0e0ca369ad5d8292e1 Mon Sep 17 00:00:00 2001 From: dcherian Date: Tue, 15 Oct 2019 16:25:41 -0600 Subject: [PATCH 2/6] Add whats-new. --- doc/whats-new.rst | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/doc/whats-new.rst b/doc/whats-new.rst index 9e14120aeb3..cf56351e44c 100644 --- a/doc/whats-new.rst +++ b/doc/whats-new.rst @@ -18,6 +18,21 @@ What's New v0.14.1 (unreleased) -------------------- +Breaking changes +~~~~~~~~~~~~~~~~ + +New functions/methods +~~~~~~~~~~~~~~~~~~~~~ + +Enhancements +~~~~~~~~~~~~ + +Bug fixes +~~~~~~~~~ + +- Fix :py:meth:`xarray.core.groupby.DataArrayGroupBy.reduce` and + :py:meth:`xarray.core.groupby.DatasetGroupBy.reduce` when reducing over multiple dimensions. + (:issue:`3402`). By `Deepak Cherian `_ .. _whats-new.0.14.0: From bf2269568076339c9e3cde7abb6369f30dd22ddf Mon Sep 17 00:00:00 2001 From: dcherian Date: Tue, 15 Oct 2019 20:21:51 -0600 Subject: [PATCH 3/6] Use is_scalar instead --- xarray/core/groupby.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xarray/core/groupby.py b/xarray/core/groupby.py index 347f9817e77..1115872d7bb 100644 --- a/xarray/core/groupby.py +++ b/xarray/core/groupby.py @@ -15,6 +15,7 @@ from .utils import ( either_dict_or_kwargs, hashable, + is_scalar, maybe_wrap_array, peek_at, safe_cast_to_index, @@ -24,7 +25,7 @@ def check_reduce_dims(reduce_dims, dimensions): - if isinstance(reduce_dims, str): + if is_scalar(reduce_dims): reduce_dims = [reduce_dims] if reduce_dims is not ALL_DIMS and any( From 4ccdc5d74b8c919778bacf1c5bd40a092b00a877 Mon Sep 17 00:00:00 2001 From: dcherian Date: Tue, 15 Oct 2019 20:49:27 -0600 Subject: [PATCH 4/6] bugfix --- xarray/core/groupby.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/xarray/core/groupby.py b/xarray/core/groupby.py index 1115872d7bb..1bd13ce02a4 100644 --- a/xarray/core/groupby.py +++ b/xarray/core/groupby.py @@ -25,16 +25,14 @@ def check_reduce_dims(reduce_dims, dimensions): - if is_scalar(reduce_dims): - reduce_dims = [reduce_dims] - - if reduce_dims is not ALL_DIMS and any( - [dim not in dimensions for dim in reduce_dims] - ): - raise ValueError( - "cannot reduce over dimensions %r. expected either xarray.ALL_DIMS to reduce over all dimensions or one or more of %r." - % (reduce_dims, dimensions) - ) + if reduce_dims is not ALL_DIMS: + if is_scalar(reduce_dims): + reduce_dims = [reduce_dims] + if any([dim not in dimensions for dim in reduce_dims]): + raise ValueError( + "cannot reduce over dimensions %r. expected either xarray.ALL_DIMS to reduce over all dimensions or one or more of %r." + % (reduce_dims, dimensions) + ) def unique_value_groups(ar, sort=True): From cd5438302844706b18f48f3fa6c845e3a4739408 Mon Sep 17 00:00:00 2001 From: dcherian Date: Tue, 22 Oct 2019 15:12:26 -0600 Subject: [PATCH 5/6] fix whats-new --- doc/whats-new.rst | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/doc/whats-new.rst b/doc/whats-new.rst index 028c42cf819..e9966bf679c 100644 --- a/doc/whats-new.rst +++ b/doc/whats-new.rst @@ -21,18 +21,6 @@ v0.14.1 (unreleased) Breaking changes ~~~~~~~~~~~~~~~~ -New functions/methods -~~~~~~~~~~~~~~~~~~~~~ - -Enhancements -~~~~~~~~~~~~ - -Bug fixes -~~~~~~~~~ - -- Fix :py:meth:`xarray.core.groupby.DataArrayGroupBy.reduce` and - :py:meth:`xarray.core.groupby.DatasetGroupBy.reduce` when reducing over multiple dimensions. - (:issue:`3402`). By `Deepak Cherian `_ - Minimum cftime version is now 1.0.3. By `Deepak Cherian `_. New Features @@ -56,6 +44,10 @@ Bug fixes - Sync with cftime by removing `dayofwk=-1` for cftime>=1.0.4. By `Anderson Banihirwe `_. +- Fix :py:meth:`xarray.core.groupby.DataArrayGroupBy.reduce` and + :py:meth:`xarray.core.groupby.DatasetGroupBy.reduce` when reducing over multiple dimensions. + (:issue:`3402`). By `Deepak Cherian `_ + Documentation ~~~~~~~~~~~~~ From d464997a55d49d194528505b97eb809e5415c46e Mon Sep 17 00:00:00 2001 From: Deepak Cherian Date: Fri, 25 Oct 2019 11:19:37 -0600 Subject: [PATCH 6/6] Update xarray/core/groupby.py Co-Authored-By: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> --- xarray/core/groupby.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xarray/core/groupby.py b/xarray/core/groupby.py index c8bd784e924..62c055fed51 100644 --- a/xarray/core/groupby.py +++ b/xarray/core/groupby.py @@ -30,7 +30,7 @@ def check_reduce_dims(reduce_dims, dimensions): reduce_dims = [reduce_dims] if any([dim not in dimensions for dim in reduce_dims]): raise ValueError( - "cannot reduce over dimensions %r. expected either xarray.ALL_DIMS to reduce over all dimensions or one or more of %r." + "cannot reduce over dimensions %r. expected either '...' to reduce over all dimensions or one or more of %r." % (reduce_dims, dimensions) )