Add option to not roll coords #2360

ahuang11 · 2018-08-10T05:14:35Z

Closes roll doesn't handle periodic boundary conditions well #1875
Tests added (for all bug fixes or enhancements)
Tests passed (for all non-documentation changes)
Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

Will add the others stuff from the checklist soon.

fujiisoup

Thanks for sending a PR.

I personally like more explicit argument name, such as roll_coordinate rather than simple coords so that users easily know a boolean should be passed for this.

Any idea?

For example, a boolean argument of interpolate_na is use_coordinate.

fujiisoup · 2018-08-10T11:24:29Z

xarray/core/dataarray.py

@@ -2015,14 +2015,20 @@ def shift(self, **shifts):
        variable = self.variable.shift(**shifts)
        return self._replace(variable)

-    def roll(self, **shifts):
+    def roll(self, coords, **shifts):


This argument should have a default.

fujiisoup

Thanks. Looks good to me.
@shoyer, can you take a look?

Logic should be updated.

fujiisoup · 2018-08-12T05:16:48Z

xarray/core/dataset.py

-            var_shifts = dict((k, v) for k, v in shifts.items()
-                              if k in var.dims)
-            variables[name] = var.roll(**var_shifts)
+            if name in self.data_vars or roll_coords:


I think this logic should be updated.
All the variables in data_vars should be rolled, independent from roll_coords value.
Other variables (variables in self.coord_names) may depend on whether roll_coords is True or not.

fujiisoup · 2018-08-12T05:36:51Z

xarray/core/dataset.py

+            elif name in coord_names and roll_coords is None:
+                warnings.warn("roll_coords will be set to False in the future."
+                              " Explicitly set roll_coords to silence warning.",
+                               DeprecationWarning, stacklevel=3)


My previous comment seemed confusing...
I think your previous commit is cleaner.

Or how about something like this?

if roll_coords is None: warnings.warn("roll_coords will be set to False in the future." " Explicitly set roll_coords to silence warning.", DeprecationWarning, stacklevel=3) roll_coords = True unrolled_vars = () if roll_coords else self.coord_names variables = OrderedDict() for k, v in self.variables: if k not in unrolled_vars: variables[k] = v.roll(**coords) else: variables[k] = v

Yes, that definitely looks cleaner.

fujiisoup

Thanks. This looks nice to me.

shoyer · 2018-08-13T04:16:35Z

xarray/core/dataset.py

+        if roll_coords is None:
+            warnings.warn("roll_coords will be set to False in the future."
+                          " Explicitly set roll_coords to silence warning.",
+                          DeprecationWarning, stacklevel=3)


Use FutureWarning here, which doesn't get silenced by default and is intended for warning about future changes in behavior.

shoyer · 2018-08-13T04:20:05Z

xarray/core/dataset.py

+        if roll_coords is None:
+            warnings.warn("roll_coords will be set to False in the future."
+                          " Explicitly set roll_coords to silence warning.",
+                          DeprecationWarning, stacklevel=3)


Also -- consider using stacklevel=2 here. stacklevel=3 here would be confusing if Dataset.roll() is called directly.

I think it's probably better to incorrect indicate that the error is being raised internally inside xarray rather than to incorrectly indicate that it's being raised by some arbitrary user code.

shoyer · 2018-08-13T04:21:15Z

xarray/tests/test_dataarray.py

+
+    def test_roll_coords_none(self):
+        arr = DataArray([1, 2, 3], coords={'x': range(3)}, dims='x')
+        actual = arr.roll(x=1, roll_coords=None)


Please verify that the warning is raised, e.g.,

with pytest.raises(FutureWarning): ...

shoyer · 2018-08-13T04:23:12Z

xarray/core/dataarray.py

+            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 and sort.


What does "and sort" refer to?

shoyer · 2018-08-13T04:24:34Z

xarray/core/dataarray.py

@@ -2015,14 +2015,20 @@ def shift(self, **shifts):
        variable = self.variable.shift(**shifts)
        return self._replace(variable)

-    def roll(self, **shifts):
+    def roll(self, roll_coords=None, **shifts):


It might be nice to use utils.either_dict_or_kwargs here, to handle potential (unlikely) conflicts between the name "roll_coords" and dimension names.

Example usage:

xarray/xarray/core/dataarray.py

Line 755 in ded0a68

indexers = either_dict_or_kwargs(indexers, indexers_kwargs, 'isel')

Not too sure I understand this util; let me know if I implemented it inaccurately. (I think I got it now?)

fujiisoup

Thanks a lot :)
A few further comments, but it is almost ready.

fujiisoup · 2018-08-13T06:13:23Z

xarray/core/dataarray.py

@@ -2046,7 +2052,10 @@ def roll(self, **shifts):
        Coordinates:
          * x        (x) int64 2 0 1
        """
-        ds = self._to_temp_dataset().roll(**shifts)
+        shifts = either_dict_or_kwargs(shifts,


Thanks.
You can pass both shifts and shifts_kwargs to _to_temp_dataset().roll directly, in order to simplify the script here. It will be handled propery in dataset.roll.

fujiisoup · 2018-08-13T06:14:34Z

xarray/core/dataset.py

@@ -3389,15 +3399,27 @@ def roll(self, **shifts):
        Data variables:
            foo      (x) object 'd' 'e' 'a' 'b' 'c'
        """
+        shifts = either_dict_or_kwargs(shifts,


Maybe better to avoid unnecessary line breaks.
Simply write either_dict_or_kwargs(shifts, shifts_kwargs, 'roll')

fujiisoup · 2018-08-15T08:11:57Z

Thanks!

Add option to not roll coords

2f8a835

fujiisoup reviewed Aug 10, 2018

View reviewed changes

ahuang11 added 2 commits August 11, 2018 21:14

Rename keyword arg and add tests

2caa553

Add what's new

a2737b8

fujiisoup previously approved these changes Aug 12, 2018

View reviewed changes

Fix passing None and add more tests

0b5e7fd

fujiisoup reviewed Aug 12, 2018

View reviewed changes

Revise from comments

a4da1ca

fujiisoup reviewed Aug 12, 2018

View reviewed changes

Revise with cleaner version

896b18a

fujiisoup approved these changes Aug 12, 2018

View reviewed changes

shoyer reviewed Aug 13, 2018

View reviewed changes

ahuang11 added 2 commits August 12, 2018 22:27

Revisions based on comments

c64f726

Fix either_dict_or_kwargs

5725578

fujiisoup reviewed Aug 13, 2018

View reviewed changes

ahuang11 added 2 commits August 14, 2018 17:55

Merge branch 'master' into master

a57f794

Revisions from comments

3f9dc4a

fujiisoup merged commit cbb2aeb into pydata:master Aug 15, 2018

This was referenced Sep 27, 2018

fix:2445 #2446

Merged

roll() now won't operate on multi-dimensional arrays. #2445

Closed

TomNicholas mentioned this pull request Jul 30, 2021

Roll coords deprecation #5653

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to not roll coords #2360

Add option to not roll coords #2360

ahuang11 commented Aug 10, 2018 •

edited

Loading

fujiisoup left a comment •

edited

Loading

fujiisoup Aug 10, 2018

fujiisoup left a comment

fujiisoup Aug 12, 2018

fujiisoup Aug 12, 2018

ahuang11 Aug 12, 2018

fujiisoup left a comment

shoyer Aug 13, 2018

shoyer Aug 13, 2018

shoyer Aug 13, 2018

shoyer Aug 13, 2018

shoyer Aug 13, 2018

ahuang11 Aug 13, 2018 •

edited

Loading

fujiisoup left a comment

fujiisoup Aug 13, 2018 •

edited

Loading

fujiisoup Aug 13, 2018

fujiisoup commented Aug 15, 2018

Add option to not roll coords #2360

Add option to not roll coords #2360

Conversation

ahuang11 commented Aug 10, 2018 • edited Loading

fujiisoup left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuang11 Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

fujiisoup left a comment

Choose a reason for hiding this comment

fujiisoup Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup commented Aug 15, 2018

ahuang11 commented Aug 10, 2018 •

edited

Loading

fujiisoup left a comment •

edited

Loading

ahuang11 Aug 13, 2018 •

edited

Loading

fujiisoup Aug 13, 2018 •

edited

Loading