From 7be30017df889401ea68fa72dae99f2b3beb7023 Mon Sep 17 00:00:00 2001 From: Keewis Date: Sun, 21 Feb 2021 23:31:27 +0100 Subject: [PATCH 1/7] don't allow passing 'axis' to Dataset.reduce methods --- xarray/core/dataset.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/xarray/core/dataset.py b/xarray/core/dataset.py index 066a2f690b0..8b39d93116e 100644 --- a/xarray/core/dataset.py +++ b/xarray/core/dataset.py @@ -4656,6 +4656,12 @@ def reduce( Dataset with this object's DataArrays replaced with new DataArrays of summarized data and the indicated dimension(s) removed. """ + if "axis" in kwargs: + raise ValueError( + "passing 'axis' to Dataset reduce methods is ambiguous." + " Please use 'dim' instead." + ) + if dim is None or dim is ...: dims = set(self.dims) elif isinstance(dim, str) or not isinstance(dim, Iterable): From 3ef47e9f1924bdf389b1c74c36265321f4a351d3 Mon Sep 17 00:00:00 2001 From: Keewis Date: Sun, 21 Feb 2021 23:33:46 +0100 Subject: [PATCH 2/7] check that Dataset.reduce raises for axis kwargs --- xarray/tests/test_dataset.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/xarray/tests/test_dataset.py b/xarray/tests/test_dataset.py index c4161093837..6ad9ec35eca 100644 --- a/xarray/tests/test_dataset.py +++ b/xarray/tests/test_dataset.py @@ -4746,6 +4746,9 @@ def test_reduce(self): assert_equal(data.mean(dim=[]), data) + with pytest.raises(ValueError): + data.mean(axis=0) + def test_reduce_coords(self): # regression test for GH1470 data = xr.Dataset({"a": ("x", [1, 2, 3])}, coords={"b": 4}) From 724a69934c7a1bb6b497733b0ed918b68fbd1643 Mon Sep 17 00:00:00 2001 From: Keewis Date: Sun, 21 Feb 2021 23:37:17 +0100 Subject: [PATCH 3/7] update whats-new.rst [skip-ci] --- doc/whats-new.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/whats-new.rst b/doc/whats-new.rst index 8efb985a175..69fffa734d0 100644 --- a/doc/whats-new.rst +++ b/doc/whats-new.rst @@ -152,6 +152,8 @@ Bug fixes - Allow converting :py:class:`Dataset` or :py:class:`DataArray` objects with a ``MultiIndex`` and at least one other dimension to a ``pandas`` object (:issue:`3008`, :pull:`4442`). By `ghislainp `_. +- don't allow passing ``axis`` to :py:meth:`Dataset.reduce` methods (:issue:`3510`, :pull:`4940`). + By `Justus Magin `_. Documentation ~~~~~~~~~~~~~ From 20c1d45091a96544145cda648102c95360e5e9b0 Mon Sep 17 00:00:00 2001 From: Keewis Date: Fri, 26 Feb 2021 00:38:58 +0100 Subject: [PATCH 4/7] remove the broken axis kwarg to Dataset argmin / argmax --- xarray/core/dataset.py | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/xarray/core/dataset.py b/xarray/core/dataset.py index 8b39d93116e..a395829e054 100644 --- a/xarray/core/dataset.py +++ b/xarray/core/dataset.py @@ -6851,7 +6851,7 @@ def idxmax( ) ) - def argmin(self, dim=None, axis=None, **kwargs): + def argmin(self, dim=None, **kwargs): """Indices of the minima of the member variables. If there are multiple minima, the indices of the first one found will be @@ -6865,9 +6865,6 @@ def argmin(self, dim=None, axis=None, **kwargs): this is deprecated, in future will be an error, since DataArray.argmin will return a dict with indices for all dimensions, which does not make sense for a Dataset. - axis : int, optional - Axis over which to apply `argmin`. Only one of the 'dim' and 'axis' arguments - can be supplied. keep_attrs : bool, optional If True, the attributes (`attrs`) will be copied from the original object to the new one. If False (default), the new object will be @@ -6885,28 +6882,25 @@ def argmin(self, dim=None, axis=None, **kwargs): See Also -------- DataArray.argmin - """ - if dim is None and axis is None: + if dim is None: warnings.warn( - "Once the behaviour of DataArray.argmin() and Variable.argmin() with " - "neither dim nor axis argument changes to return a dict of indices of " - "each dimension, for consistency it will be an error to call " - "Dataset.argmin() with no argument, since we don't return a dict of " - "Datasets.", + "Once the behaviour of DataArray.argmin() and Variable.argmin() without " + "dim changes to return a dict of indices of each dimension, for " + "consistency it will be an error to call Dataset.argmin() with no argument," + "since we don't return a dict of Datasets.", DeprecationWarning, stacklevel=2, ) if ( dim is None - or axis is not None or (not isinstance(dim, Sequence) and dim is not ...) or isinstance(dim, str) ): # Return int index if single dimension is passed, and is not part of a # sequence argmin_func = getattr(duck_array_ops, "argmin") - return self.reduce(argmin_func, dim=dim, axis=axis, **kwargs) + return self.reduce(argmin_func, dim=dim, **kwargs) else: raise ValueError( "When dim is a sequence or ..., DataArray.argmin() returns a dict. " @@ -6914,7 +6908,7 @@ def argmin(self, dim=None, axis=None, **kwargs): "Dataset.argmin() with a sequence or ... for dim" ) - def argmax(self, dim=None, axis=None, **kwargs): + def argmax(self, dim=None, **kwargs): """Indices of the maxima of the member variables. If there are multiple maxima, the indices of the first one found will be @@ -6928,9 +6922,6 @@ def argmax(self, dim=None, axis=None, **kwargs): this is deprecated, in future will be an error, since DataArray.argmax will return a dict with indices for all dimensions, which does not make sense for a Dataset. - axis : int, optional - Axis over which to apply `argmax`. Only one of the 'dim' and 'axis' arguments - can be supplied. keep_attrs : bool, optional If True, the attributes (`attrs`) will be copied from the original object to the new one. If False (default), the new object will be @@ -6950,26 +6941,24 @@ def argmax(self, dim=None, axis=None, **kwargs): DataArray.argmax """ - if dim is None and axis is None: + if dim is None: warnings.warn( - "Once the behaviour of DataArray.argmax() and Variable.argmax() with " - "neither dim nor axis argument changes to return a dict of indices of " - "each dimension, for consistency it will be an error to call " - "Dataset.argmax() with no argument, since we don't return a dict of " - "Datasets.", + "Once the behaviour of DataArray.argmin() and Variable.argmin() without " + "dim changes to return a dict of indices of each dimension, for " + "consistency it will be an error to call Dataset.argmin() with no argument," + "since we don't return a dict of Datasets.", DeprecationWarning, stacklevel=2, ) if ( dim is None - or axis is not None or (not isinstance(dim, Sequence) and dim is not ...) or isinstance(dim, str) ): # Return int index if single dimension is passed, and is not part of a # sequence argmax_func = getattr(duck_array_ops, "argmax") - return self.reduce(argmax_func, dim=dim, axis=axis, **kwargs) + return self.reduce(argmax_func, dim=dim, **kwargs) else: raise ValueError( "When dim is a sequence or ..., DataArray.argmin() returns a dict. " From ff776c39e7c720173bbc5af5108831bcce220462 Mon Sep 17 00:00:00 2001 From: Keewis Date: Fri, 26 Feb 2021 01:08:18 +0100 Subject: [PATCH 5/7] remove tests which depended on axis being passed through by **kwargs --- xarray/tests/test_dataset.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/xarray/tests/test_dataset.py b/xarray/tests/test_dataset.py index 6ad9ec35eca..f6476c3511a 100644 --- a/xarray/tests/test_dataset.py +++ b/xarray/tests/test_dataset.py @@ -4929,9 +4929,6 @@ def mean_only_one_axis(x, axis): with raises_regex(TypeError, "missing 1 required positional argument: 'axis'"): ds.reduce(mean_only_one_axis) - with raises_regex(TypeError, "non-integer axis"): - ds.reduce(mean_only_one_axis, axis=["x", "y"]) - def test_reduce_no_axis(self): def total_sum(x): return np.sum(x.flatten()) @@ -4941,9 +4938,6 @@ def total_sum(x): actual = ds.reduce(total_sum) assert_identical(expected, actual) - with raises_regex(TypeError, "unexpected keyword argument 'axis'"): - ds.reduce(total_sum, axis=0) - with raises_regex(TypeError, "unexpected keyword argument 'axis'"): ds.reduce(total_sum, dim="x") From b626511e0e21a986afc639ece9ec6a3bb20e2dbb Mon Sep 17 00:00:00 2001 From: Keewis Date: Fri, 26 Feb 2021 01:26:29 +0100 Subject: [PATCH 6/7] don't try to test calling numpy reduce functions on dataset objects --- xarray/tests/test_units.py | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/xarray/tests/test_units.py b/xarray/tests/test_units.py index 76dd830de23..8b7835e5da6 100644 --- a/xarray/tests/test_units.py +++ b/xarray/tests/test_units.py @@ -3972,35 +3972,6 @@ def test_repr(self, func, variant, dtype): @pytest.mark.parametrize( "func", ( - function("all"), - function("any"), - pytest.param( - function("argmax"), - marks=pytest.mark.skip( - reason="calling np.argmax as a function on xarray objects is not " - "supported" - ), - ), - pytest.param( - function("argmin"), - marks=pytest.mark.skip( - reason="calling np.argmin as a function on xarray objects is not " - "supported" - ), - ), - function("max"), - function("min"), - function("mean"), - pytest.param( - function("median"), - marks=pytest.mark.xfail(reason="median does not work with dataset yet"), - ), - function("sum"), - function("prod"), - function("std"), - function("var"), - function("cumsum"), - function("cumprod"), method("all"), method("any"), method("argmax", dim="x"), From 5cba3e008de83c616f465c6398b82389d23f9c31 Mon Sep 17 00:00:00 2001 From: keewis Date: Sun, 28 Feb 2021 19:49:08 +0100 Subject: [PATCH 7/7] Update doc/whats-new.rst Co-authored-by: Mathias Hauser --- doc/whats-new.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/whats-new.rst b/doc/whats-new.rst index 69fffa734d0..e4c6f5be4fc 100644 --- a/doc/whats-new.rst +++ b/doc/whats-new.rst @@ -152,7 +152,7 @@ Bug fixes - Allow converting :py:class:`Dataset` or :py:class:`DataArray` objects with a ``MultiIndex`` and at least one other dimension to a ``pandas`` object (:issue:`3008`, :pull:`4442`). By `ghislainp `_. -- don't allow passing ``axis`` to :py:meth:`Dataset.reduce` methods (:issue:`3510`, :pull:`4940`). +- Don't allow passing ``axis`` to :py:meth:`Dataset.reduce` methods (:issue:`3510`, :pull:`4940`). By `Justus Magin `_. Documentation