boundary options for rolling.construct #3587
Conversation
|
Hello @fujiisoup! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-12-03 05:17:40 UTC |
| @@ -206,6 +206,11 @@ def construct(self, window_dim, stride=1, fill_value=dtypes.NA): | |||
| Size of stride for the rolling window. | |||
| fill_value: optional. Default dtypes.NA | |||
| Filling value to match the dimension size. | |||
| mode: optional. Default None | |||
| One of None | 'edge' | 'reflect' | 'symmetric' | 'wrap' | |||
There was a problem hiding this comment.
Maybe the default should be constant, which would match np.pad?
| @@ -206,6 +206,11 @@ def construct(self, window_dim, stride=1, fill_value=dtypes.NA): | |||
| Size of stride for the rolling window. | |||
| fill_value: optional. Default dtypes.NA | |||
| Filling value to match the dimension size. | |||
| mode: optional. Default None | |||
There was a problem hiding this comment.
mode seems a little vague here in the context of constructing a rolling window. Maybe we could call this pad_mode or boundary_mode?
There was a problem hiding this comment.
pad_mode seems better since we should eventually have da.pad(mode="constant").
xarray/core/dask_array_ops.py
Outdated
| if version.LooseVersion(dask.__version__) < "1.7": | ||
| if mode is not None: | ||
| raise NotImplementedError( | ||
| "dask >= 1.7 is required for mode={}.".format(mode) |
There was a problem hiding this comment.
Is this the right version number for dask? I can't find a 1.7 release.
Also note that we only need to support versions of dask < 6 months old, per http://xarray.pydata.org/en/stable/installing.html. If we can just delete the old code entirely, that might be cleaner.
There was a problem hiding this comment.
Dask API page says it came at the version 1.7, but I just noticed this doc is just copied from numpy...
I'll fix it.
|
Thanks for picking this up - excited to get this in! Are you able to get it to work with dask arrays? (That was the stopper in my PR.) Or maybe you can require that dask arrays are not chunked along the rolling dimension? |
|
@fujiisoup I would eventually like to pick this one up. Can you remember what was missing? Mostly the tests? |
|
I was thinking to wait for the pad method implemented but forgot until now. The first motivation was to implement the rolling operation for the periodic coordinate, but it is not yet implemented. |
black . && mypy . && flake8whats-new.rstfor all changes andapi.rstfor new APIAdded some boundary options for rolling.construct.
Currently, the option names are inherited from
np.pad,['edge' | 'reflect' | 'symmetric' | 'wrap'].Do we want a more intuitive name, such as
periodic?