Fill gaps limited 7665

[Ockenfuss]

Improve gap filling

• Closes Interpolate_na: Rework 'limit' argument documentation/implementation #7665
• Combine current gap limiting in xarray (max_gap) with capabilities of pandas (limit_direction, limit_area) regarding filling of missing values
• Provide a common function fill_gaps() to control gap length for all filling functions (ffill/bfill/interpolate_na/fillna)

This PR involves a full implementation, documentation and corresponding tests. As discussed in #7665, a new API function fill_gaps was introduced, to avoid breaking changes of existing code and keep the argument inflation of the filling functions to a minimum.

• Closes Interpolate_na: Rework 'limit' argument documentation/implementation #7665 (also Multidimensional interpolate_na not working #9392 )
• Tests added

TBD if accepted:

• User visible changes (including notable bug fixes) are documented in whats-new.rst
• New functions/methods are listed in api.rst
• Add Example in Documentation

Deprecations

• Using limit in interpolate_na now requires numbagg or bottleneck. So far, limit worked without bottleneck, max_gap required it (not documented). Reason: max_gap and limit now share the same codebase using ffill. Since no one ever complained about the missing documentation and the error message is pretty clear and easy to resolve ("No module named 'bottleneck"), I hope this might be acceptable.

[max-sixty]

This seems extremely good!!

I left some refine-y comments, nothing preclusive.

What do others think? I don't use interpolate_na myself much so other may have views...

[max-sixty]

Refine-y but:

• are interpolate and extrapolate clearer options?
• is there a better name for the param? I'm not sure. "area" sounds 2D to me

[Ockenfuss]

True. I adopted this nomenclature from pandas. What about limit_region, does this sound less 2D?
Personally, I like the 'inside'/'outside' options, 'interpolate' sounds close to linear interpolation to me...

[max-sixty]

Any thoughts from others on the naming? Would .fill be insufficiently specific that it's filling na? Would fill_missing be clearer than fill_gaps?

[Ockenfuss]

Open to any of those. .fill sounds very concise, but maybe this is easily confused with .ffill

[max-sixty]

I think .fill could be quite nice, do others have a view?

[dcherian]

Maybe gap_filler instead, since this method does not actually fill the gaps.

I'm also wondering if its better to have a method that constructs the appropriate mask that can be used later

mask = ds.get_gap_mask(max_gap=...)
ds.ffill(...).where(~mask)

[Ockenfuss]

Good points! Just to explain:

gap_filler emphasizes the returned object type nicely! However, I choose fill_gaps because it fits the naming scheme of other object-returning functions better (e.g. rolling and coarsen are not called roller and coarser in xarray, even though the operation is not perfomed immediately and an object is returned).
Ultimately (I am a non-native english speaker) I am happy for any recommendations regarding nomenclature.
If you prefer gap_filler, I will change accordingly.

The function API is also presented as an alternative in the initial proposal. I decided to go for the object way because it is shorter (one line) and less error prone (you might easily forget the ~). If the mask is required, you can easily get it from the object:

mask=ds.gap_filler(...).mask

[headtr1ck]

Basically all the new methods are missing return types, please consider adding them.

The new class should be generic, I have added a few review comments here and there but more adoptions are needed.

[Ockenfuss]

Basically all the new methods are missing return types, please consider adding them.

The new class should be generic, I have added a few review comments here and there but more adoptions are needed.

Thanks a lot for the review of the typing! I`ll work through them asap and try to make all adoptions (probabily still need some help afterwards, though ...)

[Ockenfuss]

@headtr1ck I did my best to add type hints everywhere. Not sure how to resolve the last four remaining mypy errors...

[headtr1ck]

@headtr1ck I did my best to add type hints everywhere. Not sure how to resolve the last four remaining mypy errors...

I'm currently on holiday and it's difficult to debug these errors on the phone.
I can do it in 1-2 weeks or someone else can help out?

[max-sixty]

There are a couple of outstanding questions about the interfaces; in particular:

• forward vs back vs direction: Fill gaps limited 7665 #9402 (comment)
• some names: Fill gaps limited 7665 #9402 (comment)
• (anything else @Ockenfuss ?)

Re typing — are the interfaces typed correctly? I think they are. Assuming those are correct, I would vote that it's fine to add ignores internally (ofc the internal checks are useful tests for the external interfaces, so ideally we'd fix those)

What's the best way of resolving those outstanding questions @pydata/xarray ? I know I've been terrible on attending calls now I'm on PT, but maybe we could do it on one of those? (would be great to minimize the delay on these sorts of PRs — I think they're really valuable and would love to engender more...)

[Ockenfuss]

One more naming decision:

• Fill gaps limited 7665 #9402 (comment)
  I am on conference this week, but mostly available the next two weeks, if anyone wants to discuss. Otherwise, I will join the next dev meeting on Sep. 25.

[max-sixty]

Did you manage to discuss this this morning?

[kmuehlbauer]

Did you manage to discuss this this morning?

@max-sixty Yes, @Ockenfuss explained what decisions are to be made and encouraged to have a look here.

[Ockenfuss]

Hi @max-sixty, not too much community feedback yet :) What do you think, shall we have another look on the three open naming issues and then push this over the finish line?
I still think this is quite useful functionality for all experimentalists, where gaps of all sizes are common in measurement data.

[max-sixty]

What do you think, shall we have another look on the three open naming issues and then push this over the finish line?

Yes let's do it! I would bias for action; if folks disagree then dissent welcome.

What's remaining? Personally I think we should simplify this: #9402 (comment) + resolve any names. The external types should be correct; internally some ignores are OK...

[Ockenfuss]

Ok, I think from my side this is ready for merge. I stayed with the fill_gaps and limit_area names for now. Check out the doc build pages for fill_gaps and the GapMask object to get a feeling for it and let me know if it feels right :)

This PR leaves all existing fill function API untouched. If we want to bring some of the new limit arguments to the existing functions (like DataArray.fillna(limit=...)), this could be done later in separate PRs.

TBD:

• Update Whatsnew

[max-sixty]

Looks great, I think this is indeed very close to landing.

The one thing I'm stuck on is the concept of fill_direction="backward" ... .ffill... — can we explain what the purpose of this is? My mental model is that we're filling missing data from an edge in a single direction; I don't understand how what's going on to fill backwards in a forward fill.

(Totally fine if others get this, I'm not the gatekeeper here, but do think it's important the API makes sense...)

[Ockenfuss]

See this comment for an explanation, or just try out what happens :) For example:

da=xr.DataArray([1, np.nan, np.nan,  2], dims=['x'])
da.fill_gaps('x', limit=1, limit_direction='backward').ffill()

<xarray.DataArray (x: 4)> Size: 32B
array([ 1., nan,  1.,  2.])
Dimensions without coordinates: x

I agree, this is maybe not obvious. Here, I explained why I left it as is. If you want, I can try to keep track of the limit_direction argument upon mask creation and raise an exception, if the user applies ffill later. But this requires some tweaks in the code.

Edit: I added a sentence to the fill_gaps doc page to explain the behaviour better.

[max-sixty]

Yes, I see what it does, but (very very respectfully, with much admiration for the overall PR!), it doesn't make semantic sense to me.

When I think "backward", I mean "look behind you, is there a gap? OK now fill it". I don't see how it can mean "start one step back, and then decide which direction to continue". Why only one step?

I think we should try hard to have xarray's interfaces be understandable without much technical knowledge. (Indeed, I think there's probably a more precise way of describing my objection around edges and continuous vs. discrete, but hopefully this suffices).

Would you object to removing the limit_direction param, and deferring that to the method?