Skip to content
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

Pass kwargs to interpolate_over_time #383

Merged
merged 5 commits into from
Jan 27, 2025
Merged

Conversation

niksirbi
Copy link
Member

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Change in default behaviour

Why is this PR needed?

See #379

What does this PR do?

  • Adds **kwargs to the interpolate_over_time() function, so that additional keyword arguments can be passed to underlying methods.
  • fill_value="extrapolate" is no longer hard-coded, instead the user may alter this parameter via the aforementioned **kwargs. Henceworth, gaps at the edges of time series will not be filled by default.
  • Removes the method argument from interpolate_over_time() function signature, as this is passed verbatim to underlying functions.
  • Retains the max_gap argument, as its meaning is slightly different from DataArray.interpolate_na().
  • Adjusts the function's docstring accordingly.
  • Modifies some of the examples accordingly.

How has this PR been tested?

The aforementioned changes broke the filtering integration test, because that test was expecting interpolate_over_time() to eliminate all missing values. I altered that test by explicitly passing fill_value="extrapolate" to it, which made it pass; this additionally acts as an implicit test that kwargs are indeed being forwarded.

Is this a breaking change?

No, as previous syntaxes will still work, but it changes the default expected behaviour.

Does this PR require an update to the documentation?

Yes, the filter_and_interpolate.py and the smooth.py examples have been updated accordingly. API docs are automatically refreshed due to the changes in the docstring.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@niksirbi niksirbi linked an issue Jan 21, 2025 that may be closed by this pull request
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.80%. Comparing base (fc30b5a) to head (ccd44d4).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #383      +/-   ##
==========================================
+ Coverage   99.79%   99.80%   +0.01%     
==========================================
  Files          14       14              
  Lines         969     1019      +50     
==========================================
+ Hits          967     1017      +50     
  Misses          2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@niksirbi niksirbi changed the title add **kwargs to interpolate_over_time() Pass kwargs to interpolate_over_time() Jan 21, 2025
@niksirbi niksirbi changed the title Pass kwargs to interpolate_over_time() Pass kwargs to interpolate_over_time Jan 21, 2025
@niksirbi niksirbi marked this pull request as ready for review January 21, 2025 16:14
@niksirbi niksirbi requested a review from lochhh January 21, 2025 16:14
Copy link
Collaborator

@lochhh lochhh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @niksirbi for the quick action!
I've briefly discussed this PR with @sfmig and we both agree that the interpolation method is rather important and should remain in the function signature.
I think, for clarity, **kwargs should be the exact **kwargs that xarray.DataArray.interpolate_na(dim=None, method='linear', limit=None, use_coordinate=True, max_gap=None, keep_attrs=None, **kwargs) expects to be passed to the underlying numpy.interp or scipy.interpolate.

examples/filter_and_interpolate.py Outdated Show resolved Hide resolved
examples/filter_and_interpolate.py Outdated Show resolved Hide resolved
examples/filter_and_interpolate.py Outdated Show resolved Hide resolved
@niksirbi
Copy link
Member Author

Thanks for the review @lochhh. I was anyway unsure about the removal of method so happy to follow you and Sofía on this. I've brought method back, I've clarified the wording on **kwargs following your suggestion and tweaked the phrasing in the example.

Copy link
Collaborator

@lochhh lochhh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀 Thanks again @niksirbi!

@lochhh lochhh added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit 22fe3e1 Jan 27, 2025
28 checks passed
@lochhh lochhh deleted the expose-interpolate-kwargs branch January 27, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extrapolate points in interpolate_over_time?
2 participants