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.

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

warn if dim is passed to rolling operations. #3513

Merged
merged 3 commits into from
Nov 13, 2019
Merged

warn if dim is passed to rolling operations. #3513

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23cc7f4
warn if dim is passed to rolling operations.
dcherian Nov 12, 2019
51ab976
Update doc/whats-new.rst
dcherian Nov 12, 2019
855de10
Update xarray/core/rolling.py
dcherian Nov 12, 2019
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
3 changes: 3 additions & 0 deletions doc/whats-new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@ Bug fixes
By `Deepak Cherian <https://github.com/dcherian>`_.
- Fix error in concatenating unlabeled dimensions (:pull:`3362`).
By `Deepak Cherian <https://github.com/dcherian/>`_.
- Warn if the ``dim`` kwarg is passed to rolling operations. This is redundant since a dimension is
specified when the :py:class:``DatasetRolling`` or :py:class:``DataArrayRolling`` object is created.
dcherian marked this conversation as resolved.
Show resolved Hide resolved
(:pull:`3362`). By `Deepak Cherian <https://github.com/dcherian/>`_.

Documentation
~~~~~~~~~~~~~
Expand Down
9 changes: 9 additions & 0 deletions xarray/core/rolling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import functools
import warnings
from typing import Callable

import numpy as np
Expand Down Expand Up @@ -351,6 +352,14 @@ def _bottleneck_reduce(self, func, **kwargs):
def _numpy_or_bottleneck_reduce(
self, array_agg_func, bottleneck_move_func, **kwargs
):
if "dim" in kwargs:
warnings.warn(
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 this just be a UserWarning saying that the dim kwargs is ignored?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's definitely incorrect to pass (i.e. there's no legitimate potential use), I'd vote to raise an error in the future, rather than a warning. If someone makes a mistake, we should let them know quickly & loudly.

f"Reductions will be applied along the rolling dimension '{self.dim}'. Passing the 'dim' kwarg passed to reduction operations is deprecated and will be removed in xarray 0.16.0.",
dcherian marked this conversation as resolved.
Show resolved Hide resolved
DeprecationWarning,
stacklevel=3,
)
del kwargs["dim"]

if bottleneck_move_func is not None and not isinstance(
self.obj.data, dask_array_type
):
Expand Down
6 changes: 6 additions & 0 deletions xarray/tests/test_dataarray.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4188,6 +4188,9 @@ def test_rolling_wrapped_bottleneck(da, name, center, min_periods):
)
assert_array_equal(actual.values, expected)

with pytest.warns(DeprecationWarning, match="Reductions will be applied"):
getattr(rolling_obj, name)(dim="time")

# Test center
rolling_obj = da.rolling(time=7, center=center)
actual = getattr(rolling_obj, name)()["time"]
Expand All @@ -4203,6 +4206,9 @@ def test_rolling_wrapped_dask(da_dask, name, center, min_periods, window):
# dask version
rolling_obj = da_dask.rolling(time=window, min_periods=min_periods, center=center)
actual = getattr(rolling_obj, name)().load()
if name != "count":
with pytest.warns(DeprecationWarning, match="Reductions will be applied"):
getattr(rolling_obj, name)(dim="time")
# numpy version
rolling_obj = da_dask.load().rolling(
time=window, min_periods=min_periods, center=center
Expand Down