Fill gaps limited 7665 #9402

Ockenfuss · 2024-08-23T16:21:32Z

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.

…nate

for more information, see https://pre-commit.ci

xarray/tests/test_missing.py

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...

xarray/core/dataarray.py

max-sixty · 2024-08-23T17:55:38Z

xarray/core/dataset.py

+        limit_direction: {"forward", "backward", "both"}, default: "forward"
+            Consecutive NaNs will be filled in this direction.


How do we feel about this param vs. the ffill / bfill on the returned object? Do we need both?

I think it is actually useful to have this parameter and allow to set it independently of the filing function. Then, for example, you can do things like:

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

which effectively pulls the gap left edge towards the gap right edge:

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

I think there might be use cases for this, like constructing masks or advanced indexing arrays maybe.

Hmmm, in my (probably naive) mental model of filling, this wouldn't work like this. How would we explain in words why we get the additional 1?

I see your point... My view on this:

fill_gaps() decides which values to fill (here 1 step backward, i.e. the right edge of every gap)

The following function decides how they are filled (ffill here)

Both are two independent steps.
Maybe it is clearer to use 'left'/'right' instead of 'forward'/'backward' terminology?

limit_side: {"left", "right", "both"}, default: "both" Whether to limit the filling to the left, right, or both sides of the gap - "left": Fill only the left (lower index) side of each gap. - "right": Fill only the right (higher index) side of each gap. - "both": Fill from both sides of each gap.

I could also join this Wednesday developer meeting to get feedback on naming/API things.

I see what you mean. I personally think that's quite confusing, but 50% chance it's just me.

Do others have a view?

I'm thinking that filling is simpler than this: it starts at the edge of a gap — and then fills the gap from the edge, which is by definition backwards or forwards (because it's already at an edge so can only go one direction)...

(yes re Wednesday calls! Unfortunately it clashes with something I have so I find it tough to go, but others will be there!)

xarray/core/dataset.py

max-sixty · 2024-08-23T18:05:07Z

xarray/core/dataset.py

+            | datetime.timedelta
+        ) = None,
+        limit_direction: LimitDirectionOptions = "both",
+        limit_area: LimitAreaOptions | None = None,


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

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...

xarray/core/dataset.py

max-sixty · 2024-08-23T18:10:03Z

xarray/core/dataarray.py

@@ -3766,6 +3769,160 @@ def bfill(self, dim: Hashable, limit: int | None = None) -> Self:

        return bfill(self, dim, limit=limit)

+    def fill_gaps(


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?

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

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

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)

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

xarray/core/missing.py

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.

xarray/core/missing.py

xarray/core/dataarray.py

for more information, see https://pre-commit.ci

Ockenfuss · 2024-08-24T20:50:38Z

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 ...)

- Allows for more flexibility, e.g. optional dim arguments - Better static typing

for more information, see https://pre-commit.ci

xarray/core/dataarray.py

Ockenfuss · 2024-08-26T12:55:40Z

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

headtr1ck · 2024-08-30T16:57:44Z

@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?

headtr1ck · 2024-08-26T13:53:44Z

xarray/core/missing.py

+        self.content = content
+        self.mask = mask
+        self.dim = dim
+        self.dim = dim


Suggested change

self.dim = dim

headtr1ck · 2024-08-30T17:05:02Z

xarray/core/missing.py

+    # Calculate individual masks
+    masks = []
+    if need_limit_mask:
+        masks.append(


I think it is difficult to tell mypy that limit cannot be a Hashable here do to the dynamic way of how it is handled.
Maybe the best course of action is to add an assertion (not sure which exactly, maybe assert not utils.is_hashable(limit), but maybe this is too restrictive?)
Easier might be to add a #type: ignore[arg-type] and a little comment as to why.

max-sixty · 2024-09-10T19:22:37Z

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 · 2024-09-11T16:50:29Z

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 · 2024-09-25T23:09:17Z

Did you manage to discuss this this morning?

kmuehlbauer · 2024-09-26T05:37:11Z

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 and others added 9 commits August 22, 2024 23:58

Introduce new arguments limit_direction, limit_area, limit_use coordi…

bbfc476

…nate

Use internal broadcasting and transpose instead of ones_like

16cdf30

Typo: Default False in doc for limit_use_coordinates

43b7165

[pre-commit.ci] auto fixes from pre-commit.com hooks

d46baa4

for more information, see https://pre-commit.ci

Towards masked implementation

1fb7795

Working fill_gaps implementation

878e6bb

Remove keep_attrs from docstring of filling functions

1ac5e9c

Fix typos, undo empty spaces, remove temporarily introduced arguments

97b00a4

[pre-commit.ci] auto fixes from pre-commit.com hooks

1626489

for more information, see https://pre-commit.ci

Ockenfuss mentioned this pull request Aug 23, 2024

Interpolate na: Fix #7665 and introduce arguments similar to pandas #8577

Closed

3 tasks

max-sixty reviewed Aug 23, 2024

View reviewed changes

xarray/tests/test_missing.py Show resolved Hide resolved

max-sixty approved these changes Aug 23, 2024

View reviewed changes

headtr1ck reviewed Aug 23, 2024

View reviewed changes

Ockenfuss and others added 4 commits August 24, 2024 21:13

Add line break for readability

b3d70d6

Enforce kwargs to be passed by name

6b4c0f7

Keep_Attrs: Default to True

84fe728

[pre-commit.ci] auto fixes from pre-commit.com hooks

3bbd6da

for more information, see https://pre-commit.ci

Ockenfuss and others added 7 commits August 25, 2024 16:08

Explicitly add fill functions in GapMask object

3ec34bf

- Allows for more flexibility, e.g. optional dim arguments - Better static typing

[pre-commit.ci] auto fixes from pre-commit.com hooks

a64809a

for more information, see https://pre-commit.ci

Add type hints to most arguments, return types

92c6b2a

[pre-commit.ci] auto fixes from pre-commit.com hooks

5452df1

for more information, see https://pre-commit.ci

Fix accidental double pasting of arguments

2f53449

Fix more mypy errors

3696d63

Bottleneck is required for limit functionality

d5d56ae

max-sixty reviewed Aug 26, 2024

View reviewed changes

xarray/core/dataarray.py Outdated Show resolved Hide resolved

Ockenfuss added 2 commits August 26, 2024 10:21

Docs: Require numbagg or bottleneck for ffill/bfill/fill_gaps

7570b62

Rework index conversion to have consistent typing

f19f626

keewis linked an issue Aug 26, 2024 that may be closed by this pull request

Multidimensional interpolate_na not working #9392

Open

headtr1ck reviewed Aug 30, 2024

View reviewed changes

Add new method to api.rst

b5025ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fill gaps limited 7665 #9402

Fill gaps limited 7665 #9402

Ockenfuss commented Aug 23, 2024 •

edited

Loading

max-sixty left a comment

max-sixty Aug 23, 2024

Ockenfuss Aug 24, 2024 •

edited

Loading

max-sixty Aug 25, 2024

Ockenfuss Aug 25, 2024

Ockenfuss Aug 25, 2024

max-sixty Aug 26, 2024

max-sixty Aug 26, 2024

max-sixty Aug 23, 2024

Ockenfuss Aug 24, 2024

max-sixty Aug 23, 2024

Ockenfuss Aug 24, 2024

max-sixty Aug 26, 2024

dcherian Sep 3, 2024 •

edited

Loading

Ockenfuss Sep 4, 2024 •

edited

Loading

headtr1ck left a comment

Ockenfuss commented Aug 24, 2024

Ockenfuss commented Aug 26, 2024

headtr1ck commented Aug 30, 2024

headtr1ck Aug 26, 2024

headtr1ck Aug 30, 2024 •

edited

Loading

max-sixty commented Sep 10, 2024 •

edited

Loading

Ockenfuss commented Sep 11, 2024 •

edited

Loading

max-sixty commented Sep 25, 2024

kmuehlbauer commented Sep 26, 2024

		limit_direction: {"forward", "backward", "both"}, default: "forward"
		Consecutive NaNs will be filled in this direction.

		@@ -3766,6 +3769,160 @@ def bfill(self, dim: Hashable, limit: int \| None = None) -> Self:

		return bfill(self, dim, limit=limit)

		def fill_gaps(

Fill gaps limited 7665 #9402

Are you sure you want to change the base?

Fill gaps limited 7665 #9402

Conversation

Ockenfuss commented Aug 23, 2024 • edited Loading

Improve gap filling

Deprecations

max-sixty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ockenfuss Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcherian Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Ockenfuss Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

headtr1ck left a comment

Choose a reason for hiding this comment

Ockenfuss commented Aug 24, 2024

Ockenfuss commented Aug 26, 2024

headtr1ck commented Aug 30, 2024

Choose a reason for hiding this comment

headtr1ck Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

max-sixty commented Sep 10, 2024 • edited Loading

Ockenfuss commented Sep 11, 2024 • edited Loading

max-sixty commented Sep 25, 2024

kmuehlbauer commented Sep 26, 2024

Ockenfuss commented Aug 23, 2024 •

edited

Loading

Ockenfuss Aug 24, 2024 •

edited

Loading

dcherian Sep 3, 2024 •

edited

Loading

Ockenfuss Sep 4, 2024 •

edited

Loading

headtr1ck Aug 30, 2024 •

edited

Loading

max-sixty commented Sep 10, 2024 •

edited

Loading

Ockenfuss commented Sep 11, 2024 •

edited

Loading