-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interpolate_na: Rework 'limit' argument documentation/implementation #7665
Comments
Two comments if the pandas arguments are introduced:
|
Do you have any thoughts on this? I think with the following signature, the function will be backward compatible:
|
Thanks for raising this issue @Ockenfuss and thoroughly documenting the behavior.
Agreed that is surprising and seems like a bug!
A pull request would be most welcome @Ockenfuss - scoped to either just improve the documentation or with your preferred API change. We can discuss the preferred syntax in the pull request. |
After the Xarray meeting today (with @keewis and @dcherian) we discussed the preferred option for moving forward with cleaning up the |
Some more results from the discussion:
Here are my proposals: Masking function Signature: namask('t', limit=3, limit_coords=3h, max_gap=3, max_gap_coords=3h) #in line with pandas/current xarray
namask('t', limit=3, cutoff=3h, gap_limit=3, gap_cutoff=3h) #cutoff for coordinates, limit for index For direction control: namask(limit_direction=forward/backward/both, limit_area=outside/inside/None) #regardless of index or coordinate Possible API: Making it an object: da.namask(...) #masking object
da.namask(...).fillna(123) #return dataarray
da.namask(...).mask #get mask only Making it a method: mask=da.namask(...)
da.fillna(123).where(~mask) #Invalid=True to match numpy behaviour? How to deal with existing methods:
Anything else: |
What is your issue?
Currently, the 'limit' argument of
interpolate_na
shows some counterintuitive/undocumented behaviour.Take the following example:
This will produce the following result:
Two things are surprising, in my opinion:
1
at the beginning is far from any of the given valuesComparison to pandas
Similar behaviour can be created using pandas with the following arguments:
Output
This is equivalent to the current xarray behaviour, except there is no
1
at the beginning.Cause
Currently, the fill mask in xarray is implemented using a rolling window operation, where values outside the array are assumed to be valid (therefore the
1
). Seexarray.core.missing._get_valid_fill_mask
Possible Solutions
Boundary Issue
Concerning the
1
at the beginning: I think this should be considered a bug. It is likely not what you would expect if you specify a limit. As stated, pandas does not create it as well.Asymmetric Filling
Concerning the asymmetric filling, I see two options:
limit_direction
is controlling the fill direction (left, right or both).limit_area
effectively controls if we only do interpolation or allow for extrapolation as well.What do you think?
The text was updated successfully, but these errors were encountered: