Roll coords deprecation (#5653)

* changed default

* updated tests

* updated whats-new

* Fix mypy

* Update xarray/core/dataarray.py

Co-authored-by: Mathias Hauser <[email protected]>

* Update xarray/core/dataarray.py

Co-authored-by: Mathias Hauser <[email protected]>

* Update xarray/core/dataarray.py

Co-authored-by: Mathias Hauser <[email protected]>

* Update xarray/core/dataset.py

Co-authored-by: Mathias Hauser <[email protected]>

* Update xarray/core/dataset.py

Co-authored-by: Mathias Hauser <[email protected]>

* tidy up specification of default argument in docstring

* isort linting

* Update xarray/core/indexes.py

Co-authored-by: Maximilian Roos <[email protected]>
Co-authored-by: Mathias Hauser <[email protected]>
Co-authored-by: Deepak Cherian <[email protected]>

Author: 4 people

doc/whats-new.rst

@@ -56,9 +56,12 @@ Breaking changes
 Deprecations
 ~~~~~~~~~~~~
 
+- Set the default argument for `roll_coords` to `False` for :py:meth:`DataArray.roll`
+  and :py:meth:`Dataset.roll`. (:pull:`5653`)
+  By `Tom Nicholas <https://github.com/TomNicholas>`_.
 - :py:meth:`xarray.open_mfdataset` will now error instead of warn when a value for ``concat_dim`` is
-  passed alongside ``combine='by_coords'``. By `Tom Nicholas <https://github.com/TomNicholas>`_.
-
+  passed alongside ``combine='by_coords'``.
+  By `Tom Nicholas <https://github.com/TomNicholas>`_.
 
 Bug fixes
 ~~~~~~~~~

xarray/core/dataarray.py

@@ -3169,11 +3169,14 @@ def shift(
         fill_value: Any = dtypes.NA,
         **shifts_kwargs: int,
     ) -> "DataArray":
-        """Shift this array by an offset along one or more dimensions.
+        """Shift this DataArray by an offset along one or more dimensions.
 
-        Only the data is moved; coordinates stay in place. Values shifted from
-        beyond array bounds are replaced by NaN. This is consistent with the
-        behavior of ``shift`` in pandas.
+        Only the data is moved; coordinates stay in place. This is consistent
+        with the behavior of ``shift`` in pandas.
+
+        Values shifted from beyond array bounds will appear at one end of
+        each dimension, which are filled according to `fill_value`. For periodic
+        offsets instead see `roll`.
 
         Parameters
         ----------
@@ -3212,12 +3215,15 @@ def shift(
 
     def roll(
         self,
-        shifts: Mapping[Any, int] = None,
-        roll_coords: bool = None,
+        shifts: Mapping[Hashable, int] = None,
+        roll_coords: bool = False,
         **shifts_kwargs: int,
     ) -> "DataArray":
         """Roll this array by an offset along one or more dimensions.
 
+        Unlike shift, roll treats the given dimensions as periodic, so will not
+        create any missing values to be filled.
+
         Unlike shift, roll may rotate all variables, including coordinates
         if specified. The direction of rotation is consistent with
         :py:func:`numpy.roll`.
@@ -3228,12 +3234,9 @@ def roll(
             Integer offset to rotate each of the given dimensions.
             Positive offsets roll to the right; negative offsets roll to the
             left.
-        roll_coords : bool
-            Indicates whether to roll the coordinates by the offset
-            The current default of roll_coords (None, equivalent to True) is
-            deprecated and will change to False in a future version.
-            Explicitly pass roll_coords to silence the warning.
-        **shifts_kwargs
+        roll_coords : bool, default: False
+            Indicates whether to roll the coordinates by the offset too.
+        **shifts_kwargs : {dim: offset, ...}, optional
             The keyword arguments form of ``shifts``.
             One of shifts or shifts_kwargs must be provided.
 

xarray/core/dataset.py

@@ -1097,7 +1097,7 @@ def _replace(
         coord_names: Set[Hashable] = None,
         dims: Dict[Any, int] = None,
         attrs: Union[Dict[Hashable, Any], None, Default] = _default,
-        indexes: Union[Dict[Any, Index], None, Default] = _default,
+        indexes: Union[Dict[Hashable, Index], None, Default] = _default,
         encoding: Union[dict, None, Default] = _default,
         inplace: bool = False,
     ) -> "Dataset":
@@ -5866,12 +5866,22 @@ def diff(self, dim, n=1, label="upper"):
         else:
             return difference
 
-    def shift(self, shifts=None, fill_value=dtypes.NA, **shifts_kwargs):
+    def shift(
+        self,
+        shifts: Mapping[Hashable, int] = None,
+        fill_value: Any = dtypes.NA,
+        **shifts_kwargs: int,
+    ) -> "Dataset":
+
         """Shift this dataset by an offset along one or more dimensions.
 
         Only data variables are moved; coordinates stay in place. This is
         consistent with the behavior of ``shift`` in pandas.
 
+        Values shifted from beyond array bounds will appear at one end of
+        each dimension, which are filled according to `fill_value`. For periodic
+        offsets instead see `roll`.
+
         Parameters
         ----------
         shifts : mapping of hashable to int
@@ -5926,80 +5936,87 @@ def shift(self, shifts=None, fill_value=dtypes.NA, **shifts_kwargs):
 
         return self._replace(variables)
 
-    def roll(self, shifts=None, roll_coords=None, **shifts_kwargs):
+    def roll(
+        self,
+        shifts: Mapping[Hashable, int] = None,
+        roll_coords: bool = False,
+        **shifts_kwargs: int,
+    ) -> "Dataset":
         """Roll this dataset by an offset along one or more dimensions.
 
-        Unlike shift, roll may rotate all variables, including coordinates
+        Unlike shift, roll treats the given dimensions as periodic, so will not
+        create any missing values to be filled.
+
+        Also unlike shift, roll may rotate all variables, including coordinates
         if specified. The direction of rotation is consistent with
         :py:func:`numpy.roll`.
 
         Parameters
         ----------
-        shifts : dict, optional
+        shifts : mapping of hashable to int, optional
             A dict with keys matching dimensions and values given
             by integers to rotate each of the given dimensions. Positive
             offsets roll to the right; negative offsets roll to the left.
-        roll_coords : bool
-            Indicates whether to  roll the coordinates by the offset
-            The current default of roll_coords (None, equivalent to True) is
-            deprecated and will change to False in a future version.
-            Explicitly pass roll_coords to silence the warning.
+        roll_coords : bool, default: False
+            Indicates whether to roll the coordinates by the offset too.
         **shifts_kwargs : {dim: offset, ...}, optional
             The keyword arguments form of ``shifts``.
             One of shifts or shifts_kwargs must be provided.
+
         Returns
         -------
         rolled : Dataset
-            Dataset with the same coordinates and attributes but rolled
-            variables.
+            Dataset with the same attributes but rolled data and coordinates.
 
         See Also
         --------
         shift
 
         Examples
         --------
-        >>> ds = xr.Dataset({"foo": ("x", list("abcde"))})
+        >>> ds = xr.Dataset({"foo": ("x", list("abcde"))}, coords={"x": np.arange(5)})
         >>> ds.roll(x=2)
         <xarray.Dataset>
         Dimensions:  (x: 5)
-        Dimensions without coordinates: x
+        Coordinates:
+          * x        (x) int64 0 1 2 3 4
         Data variables:
             foo      (x) <U1 'd' 'e' 'a' 'b' 'c'
+
+        >>> ds.roll(x=2, roll_coords=True)
+        <xarray.Dataset>
+        Dimensions:  (x: 5)
+        Coordinates:
+          * x        (x) int64 3 4 0 1 2
+        Data variables:
+            foo      (x) <U1 'd' 'e' 'a' 'b' 'c'
+
         """
         shifts = either_dict_or_kwargs(shifts, shifts_kwargs, "roll")
         invalid = [k for k in shifts if k not in self.dims]
         if invalid:
             raise ValueError(f"dimensions {invalid!r} do not exist")
 
-        if roll_coords is None:
-            warnings.warn(
-                "roll_coords will be set to False in the future."
-                " Explicitly set roll_coords to silence warning.",
-                FutureWarning,
-                stacklevel=2,
-            )
-            roll_coords = True
-
         unrolled_vars = () if roll_coords else self.coords
 
         variables = {}
-        for k, v in self.variables.items():
+        for k, var in self.variables.items():
             if k not in unrolled_vars:
-                variables[k] = v.roll(
-                    **{k: s for k, s in shifts.items() if k in v.dims}
+                variables[k] = var.roll(
+                    shifts={k: s for k, s in shifts.items() if k in var.dims}
                 )
             else:
-                variables[k] = v
+                variables[k] = var
 
         if roll_coords:
-            indexes = {}
-            for k, v in self.xindexes.items():
+            indexes: Dict[Hashable, Index] = {}
+            idx: pd.Index
+            for k, idx in self.xindexes.items():
                 (dim,) = self.variables[k].dims
                 if dim in shifts:
-                    indexes[k] = roll_index(v, shifts[dim])
+                    indexes[k] = roll_index(idx, shifts[dim])
                 else:
-                    indexes[k] = v
+                    indexes[k] = idx
         else:
             indexes = dict(self.xindexes)
 

xarray/core/indexes.py

@@ -5,6 +5,7 @@
     Dict,
     Hashable,
     Iterable,
+    Iterator,
     Mapping,
     Optional,
     Sequence,
@@ -449,7 +450,7 @@ class Indexes(collections.abc.Mapping):
 
     __slots__ = ("_indexes",)
 
-    def __init__(self, indexes):
+    def __init__(self, indexes: Mapping[Any, Union[pd.Index, Index]]) -> None:
         """Not for public consumption.
 
         Parameters
@@ -459,7 +460,7 @@ def __init__(self, indexes):
         """
         self._indexes = indexes
 
-    def __iter__(self):
+    def __iter__(self) -> Iterator[pd.Index]:
         return iter(self._indexes)
 
     def __len__(self):
@@ -468,7 +469,7 @@ def __len__(self):
     def __contains__(self, key):
         return key in self._indexes
 
-    def __getitem__(self, key):
+    def __getitem__(self, key) -> pd.Index:
         return self._indexes[key]
 
     def __repr__(self):

xarray/tests/test_dataarray.py

@@ -3378,19 +3378,10 @@ def test_roll_coords(self):
 
     def test_roll_no_coords(self):
         arr = DataArray([1, 2, 3], coords={"x": range(3)}, dims="x")
-        actual = arr.roll(x=1, roll_coords=False)
+        actual = arr.roll(x=1)
         expected = DataArray([3, 1, 2], coords=[("x", [0, 1, 2])])
         assert_identical(expected, actual)
 
-    def test_roll_coords_none(self):
-        arr = DataArray([1, 2, 3], coords={"x": range(3)}, dims="x")
-
-        with pytest.warns(FutureWarning):
-            actual = arr.roll(x=1, roll_coords=None)
-
-        expected = DataArray([3, 1, 2], coords=[("x", [2, 0, 1])])
-        assert_identical(expected, actual)
-
     def test_copy_with_data(self):
         orig = DataArray(
             np.random.random(size=(2, 2)),

xarray/tests/test_dataset.py

@@ -5098,25 +5098,13 @@ def test_roll_no_coords(self):
         coords = {"bar": ("x", list("abc")), "x": [-4, 3, 2]}
         attrs = {"meta": "data"}
         ds = Dataset({"foo": ("x", [1, 2, 3])}, coords, attrs)
-        actual = ds.roll(x=1, roll_coords=False)
+        actual = ds.roll(x=1)
 
         expected = Dataset({"foo": ("x", [3, 1, 2])}, coords, attrs)
         assert_identical(expected, actual)
 
         with pytest.raises(ValueError, match=r"dimensions"):
-            ds.roll(abc=321, roll_coords=False)
-
-    def test_roll_coords_none(self):
-        coords = {"bar": ("x", list("abc")), "x": [-4, 3, 2]}
-        attrs = {"meta": "data"}
-        ds = Dataset({"foo": ("x", [1, 2, 3])}, coords, attrs)
-
-        with pytest.warns(FutureWarning):
-            actual = ds.roll(x=1, roll_coords=None)
-
-        ex_coords = {"bar": ("x", list("cab")), "x": [2, -4, 3]}
-        expected = Dataset({"foo": ("x", [3, 1, 2])}, ex_coords, attrs)
-        assert_identical(expected, actual)
+            ds.roll(abc=321)
 
     def test_roll_multidim(self):
         # regression test for 2445