rolling: periodic

[mathause]

• Closes rolling: allow control over padding #2007
• 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

---

Ok, this was easier to do than initially thought, we can use np.pad(a, pads, mode='wrap') in nputils.rolling_window. However, I'm not sure if that is enough already^*.

I added an initial test, but could use some pointers where else you want this to be tested.

Questions:

• is fill_value='periodic' a good api?
• should the fill_value keyvalue be ported to rolling?
• should this also be mentioned in the docs for rolling (I only learned about rolling.construct yesterday)

---

^*rolling is present incore/dataset.py, core/dataarray.py, core/variable.py, core/rolling.py, core/dask_array_ops.py, core/nputils.py, core/ops.py, core/common.py, core/missing.py, and core/duck_array_ops.py that can be a bit daunting...

[mathause]

We would probably have to test how min_periods and periodic play together..., see #2010

---

edit: or not, as min_periods is not used in construct

[stickler-ci]

W292 no newline at end of file

[fujiisoup]

Thanks for sending a PR.

is fill_value='periodic' a good api?

Maybe better API would be boundary='peiodic' (see comments), but changing the public API is not an easy task.
Let's keep it now as is and decide it later.

should the fill_value keyvalue be ported to rolling?

I don't think so. I think we can keep rolling as similar to that of pandas,
and this option should be specific to construct method.

should this also be mentioned in the docs for rolling (I only learned about rolling.construct yesterday)

I think at least we need to add this to the docstring of construct.

[fujiisoup]

Some comments.

• We need to also support a dask array.
  Can you update dask_array_op.rolling_window? Maybe this page would help for the implementation.

We use this here

xarray/xarray/core/dask_array_ops.py

Lines 78 to 79 in 7c2c43c

	# create ghosted arrays
	ag = da.ghost.ghost(a, depth=depth, boundary=boundary)

I suspect this would be already working, but it would be nice if we can add some test cases.

• Maybe we can also add boundary='reflect' option to make this as similar to dask.ghost.

[mathause]

Thanks for the discussion. I could not get it to work with dask - the problem is the 'padding before ghosting'.

xarray/xarray/core/dask_array_ops.py

Lines 60 to 62 in 7c2c43c

	# pad_size becomes more than 0 when the ghosted array is smaller than
	# needed. In this case, we need to enlarge the original array by padding
	# before ghosting.

I see no way how to get the boundary='periodic' working afterwards...

[fujiisoup]

Ah, you are right.
The only way to realize rolling.periodic with dask would be to manually attach a chunk in the other side to the boundaries, construct rolling window array and then remove unnecessary boundaries by slicing.

[raybellwaves]

I may pick up this PR but will need some hand holding as i'm not familiar with dask.

I want to make sure I can do periodic rolling with dask in a simple script first but it needs work: https://gist.github.com/raybellwaves/621faaf195c0f4ed010b7b0dfee8605a

[rabernat]

Rather than letting this PR lie dormant, maybe we should just move forward without dask support and raise an appropriate error if the user tries it with dask arrays.

[shoyer]

It might make sense to start with the higher level pad() with periodic boundary conditions first (#2605). Then rolling with periodic boundary conditions is a simple matter of padding, then rolling.

[shoyer]

• is fill_value='periodic' a good api?

fill_value='periodic' almost suggests padding with the string 'periodic'. Maybe padding='periodic' vs padding=None (default)?