issue_comments

Great, thanks @dcherian !

I'm going to merge this. Happy to make any other changes.

Thanks for all your work @dcherian! Did a quick test with some real-world timeseries data I've been wanting to use with max_gap and it looks good to me. I will definitely be using this in the future! 👍

This could use another round of testing / review

(cc @dnowacki-usgs @stefraynaud @max-sixty )

Thanks @dnowacki-usgs that's a nice test. I think the right fix is to make index a Variable so that we get automatic broadcasting.

Things left to do (or at least add xfail tests + errors): - [x] support for cftime indexes and offsets - [x] what to do when use_coordinate=False - [x] what to do with unlabeled dimensions - [x] document convention for gap length - [x] add examples to docs

Done for now. Ready for final review / testing.

Thanks so much for taking this on. Not sure how I missed the notification that you had done so.

@dcherian I checked out your branch and did some testing and found that testing this against dim="x" fails with a ValueError. ```python arr = np.array([ [np.nan, np.nan, np.nan, 1, 2, 3, 4, np.nan, 6, 7, np.nan, np.nan, np.nan, 11, np.nan, np.nan, ], [np.nan, np.nan, np.nan, 1, 2, 3, np.nan, np.nan, 6, 7, np.nan, np.nan, np.nan, 11, np.nan, np.nan, ], [np.nan, np.nan, np.nan, 1, 2, 3, np.nan, np.nan, 6, 7, np.nan, np.nan, np.nan, 11, np.nan, np.nan, ], [np.nan, np.nan, np.nan, 1, 2, 3, 4, np.nan, 6, 7, np.nan, np.nan, np.nan, 11, np.nan, np.nan, ], ])

da = xr.DataArray(arr, dims=["x", "y"], coords={"x": np.arange(arr.shape[0]), "y": np.arange(arr.shape[1])})

actual = da.interpolate_na("y", max_gap=2) # this works as in your test example actual = da.interpolate_na("x", max_gap=2) # this fails Error:

actual = da.interpolate_na("x", max_gap=2) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/dnowacki/Documents/xarray/xarray/core/dataarray.py", line 2026, in interpolate_na **kwargs File "/Users/dnowacki/Documents/xarray/xarray/core/missing.py", line 302, in interp_na nan_block_lengths = _get_nan_block_lengths(self, dim, index) File "/Users/dnowacki/Documents/xarray/xarray/core/missing.py", line 23, in _get_nan_block_lengths arange = ones_like(obj) * index + (index[1] - index[0]) File "/Users/dnowacki/Documents/xarray/xarray/core/dataarray.py", line 2500, in func if not reflexive File "/Users/dnowacki/Documents/xarray/xarray/core/variable.py", line 1884, in func if not reflexive ValueError: operands could not be broadcast together with shapes (4,16) (4,) ```

I fixed it by changing this line https://github.com/dcherian/xarray/blob/8899f05a13533e10c8972836afda089b5b792ca0/xarray/core/missing.py#L23 to this: python if index.shape[0] == obj.shape[0]: arange = ones_like(obj) * index[:, None] + (index[1] - index[0]) else: arange = ones_like(obj) * index + (index[1] - index[0]) Does this seem reasonable?

yes, I think you are right. I was thinking that it would be nice to have the number-of-nan-points and gap-length metrics converge for uniformly spaced coordinates but I don't think that's possible in any sensible way.

IIUC, and please correct me if I'm wrong, the pandas version counts points rather than the distance between locations. Ideally we'd be able to do both, but even if we can only have one working correctly, that would be v good

The thing I find weird is that for

<xarray.DataArray (y: 9)> array([nan, nan, nan, 1., nan, nan, 4., nan, nan]) Coordinates: * y (y) int64 0 1 2 3 4 5 6 7 8

the center gap's length = 7-4 = 3 which is the number of NaNs + 1. But maybe this is OK.

1. We should check what that pandas PR does and align with that
2. interp calls scipy.interpolate.interp which does do extrapolation, so we should figure out a sensible solution for the edges (extrapolating coordinates using the first and last spacing seems reasonable to me).

@stefraynaud I don't have time to work on this now. Please feel free to modify this and open a new PR. You could try to push to this branch but I'm not sure it will work.

Thanks @stefraynaud . I'm having trouble figuring out defining the length of a gap in the irregular coordinate case.

e.g.

da4 = xr.DataArray([np.nan, np.nan, np.nan, 1, np.nan, np.nan, 4, np.nan, np.nan], dims=["y"], coords={"y": [0, 2, 5, 6, 7, 8, 10, 12, 14]})

<xarray.DataArray (y: 9)> array([nan, nan, nan, 1., nan, nan, 4., nan, nan]) Coordinates: * y (y) int64 0 2 5 6 7 8 10 12 14

What is the length of these three gaps given that xarray doesn't have any understanding of grids?

@dcherian In your example, as said @max-sixty, the middle gap has a length of 10-6=4. The length gaps at the edges cannot be computed but it doesn't matter, and the algo should work as when simply counting the nans.

I'll have a look the code, maybe for a new PR after this one.

OK added test and now raises error for irregularly spaced coordinates. I agree that this should be good for now.

I think using locations rather than counts would be great, but harder and doesn't have to be part of this PR.

In the example above, it looks like 1 is aligned with 6 and 4 with 10, so the gap in locations along the y dimension would be 4?

Thanks @stefraynaud . I'm having trouble figuring out defining the length of a gap in the irregular coordinate case.

e.g.

da4 = xr.DataArray([np.nan, np.nan, np.nan, 1, np.nan, np.nan, 4, np.nan, np.nan], dims=["y"], coords={"y": [0, 2, 5, 6, 7, 8, 10, 12, 14]})

<xarray.DataArray (y: 9)> array([nan, nan, nan, 1., nan, nan, 4., nan, nan]) Coordinates: * y (y) int64 0 2 5 6 7 8 10 12 14

What is the length of these three gaps given that xarray doesn't have any understanding of grids?

Nice feature. How about adding the support max gaps expressed in physical units, since coordinates may be irregular?

:+1:

As per the pandas issue, sounds like max_gap is consensus

OK it looks like that PR is alive again, so maybe it's good to keep the same kwarg?

Ah, agree we should align. I'm really not keen on that name but yes on balance; unless they're open to changing

Thanks @max-sixty I've updated the tests.

Any thoughts on maxgap? limit_gap? Neither clicks that well!

I pulled maxgap from this ~stalled~ Pandas PR: https://github.com/pandas-dev/pandas/pull/25141 OK it looks like that PR is alive again, so maybe it's good to keep the same kwarg?