issue_comments

nice work @fujiisoup!

@shyer, do you have further suggestions? I think this is almost ready.

@shoyer , thanks for the detailed review.

I noticed the benchmark test is still failing. After fixing this, I will merge this.

Thanks :)

@shoyer , thanks for the review.

These names are very descriptive, but unfortunately they are different for DataArray and Dataset, which means that users will need to write different code for different xarray types.

Agreed. Changed to_dataarray / to_dataset -> construct.

My final concern is about the name for these methods. Currently, you have two separate method names, to_dataset and to_dataarray(). These names are very descriptive, but unfortunately they are different for DataArray and Dataset, which means that users will need to write different code for different xarray types. I would slightly rather that we had a single method name that could be used for both, perhaps construct() or build(). Then you could write code like xarray_obj.rolling(time=100).construct(rolling_dim='window') without needing to know whether xarray_obj is a Dataset or DataArray.

What's the current status here vs #1883? Do you still want to merge that one first?

Yes. It looks I reverted to a wrong commit. I will overwrite most modifications in duck_array_ops and test_duck_array_ops after #1883 is merged..

I sent a separate PR (#1883) for nan-reduce methods. I will continue / finish up this PR after #1883 is merged.

I'll take a look, but I'm open to only fixing this nansum for numpy 1.13+ (or even requiring numpy 1.13 for rolling window arrays), as long as we don't break existing behavior on earlier versions of numpy.

On Tue, Jan 30, 2018 at 3:16 AM Keisuke Fujii notifications@github.com wrote:

Hmm... The support of nansum for object-dtype is much more difficult than I expected.

Now, I am stacked in supporting numpy<1.13. With numpy<1.13, nansum for the object type gives just a nan.

nansum scripts defined in numpy==1.13 does not help because != operator used here https://github.com/numpy/numpy/blob/6914bb41f0fb3c1ba500bae4e7d671da9536786f/numpy/lib/nanfunctions.py#L68 behaves differently in numpy<1.13.

I think our best choice is to revert this PR to 4189d71 https://github.com/pydata/xarray/commit/4189d71d9998ff83deb9a5d7035a2edaf628ae25 and left this issue to #1866 https://github.com/pydata/xarray/issues/1866.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1837#issuecomment-361562466, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1gBOXSErdmYDEYBql46Pl6DEm7uTks5tPvoTgaJpZM4Rim9O .

Hmm... The support of nansum for object-dtype is much more difficult than I expected.

Now, I am stacked in supporting numpy<1.13. With numpy<1.13, nansum for the object type gives just a nan.

nansum scripts defined in numpy==1.13 does not help because != operator used here behaves differently in numpy<1.13.

I think our best choice is to revert this PR to 4189d71 and left this issue to #1866.

Although nan-reduce methods (such as nanmean) might be sped up by replacing nan by a certain number before constructing a rolling window, this improvement brings another complexity into the code (and bottleneck already does this).

I want to finish this PR first and left such improvements to another chance. Please let me know if anyone has comments of this PR.

I think this is ready for the final review, including docs. I would like to add rolling methods to airspeed benchmark, but in another PR.

Would it make sense to add a stride argument to to_dataarray? You can always index afterwards, but that could be a nice shortcut. It will be much faster for dask, certainly. (We can also add this later if desired)

During the work, I notice a somehow unexpected behavior of rolling.

I expected that with min_periods=1 option, we will get an array without nan, It is true with center=False and also pd.rolling.

```python In [2]: da = xr.DataArray(np.arange(10), dims='x') In [3]: da.rolling(x=3, min_periods=1, center=False).sum() Out[3]: <xarray.DataArray (x: 10)> array([ 0., 1., 3., 6., 9., 12., 15., 18., 21., 24.]) Dimensions without coordinates: x

In [5]: s = pd.Series(np.arange(10)) s.rolling(3, min_periods=1, center=True).sum() Out[7]: 0 1.0 1 3.0 2 6.0 3 9.0 4 12.0 5 15.0 6 18.0 7 21.0 8 24.0 9 17.0 dtype: float64 But with `center=True`, we have a `nan` at the end. In [4]: da.rolling(x=3, min_periods=1, center=True).sum() Out[4]: <xarray.DataArray (x: 10)> array([ 1., 3., 6., 9., 12., 15., 18., 21., 24., nan]) Dimensions without coordinates: x ```

It is because we make shift operation after the rolling. https://github.com/pydata/xarray/blob/74d8318c68be884134d449afad18dfe731d48b72/xarray/core/rolling.py#L263-L269

If we pad the array before the rolling operation instead of shift, we will not get the last nan and the result would be the same to pandas. (rolling.to_dataarray('window_dim') does this).

I think this path is more intuitive. Any thoughts?