Add option to not roll coords#2360
Conversation
There was a problem hiding this comment.
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.
| return self._replace(variable) | ||
|
|
||
| def roll(self, **shifts): | ||
| def roll(self, coords, **shifts): |
There was a problem hiding this comment.
This argument should have a default.
| 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: |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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] = vThere was a problem hiding this comment.
Yes, that definitely looks cleaner.
fujiisoup
left a comment
fujiisoup
left a comment
There was a problem hiding this comment.
Thanks. This looks nice to me.
| 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) |
There was a problem hiding this comment.
Use FutureWarning here, which doesn't get silenced by default and is intended for warning about future changes in behavior.
| 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) |
There was a problem hiding this comment.
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.
|
|
||
| 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) |
There was a problem hiding this comment.
Please verify that the warning is raised, e.g.,
with pytest.raises(FutureWarning):
...
| 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. |
| return self._replace(variable) | ||
|
|
||
| def roll(self, **shifts): | ||
| def roll(self, roll_coords=None, **shifts): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Not too sure I understand this util; let me know if I implemented it inaccurately. (I think I got it now?)
fujiisoup
left a comment
fujiisoup
left a comment
There was a problem hiding this comment.
Thanks a lot :)
A few further comments, but it is almost ready.
| * x (x) int64 2 0 1 | ||
| """ | ||
| ds = self._to_temp_dataset().roll(**shifts) | ||
| shifts = either_dict_or_kwargs(shifts, |
There was a problem hiding this comment.
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.
| Data variables: | ||
| foo (x) object 'd' 'e' 'a' 'b' 'c' | ||
| """ | ||
| shifts = either_dict_or_kwargs(shifts, |
There was a problem hiding this comment.
Maybe better to avoid unnecessary line breaks.
Simply write either_dict_or_kwargs(shifts, shifts_kwargs, 'roll')
|
Thanks! |
3 participants
whats-new.rstfor all changes andapi.rstfor 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.