movement icon indicating copy to clipboard operation
movement copied to clipboard

Extended the interpolate_over_time to bfill, ffill, nearest and constant functionality

Open angkul07 opened this issue 9 months ago • 8 comments

Description

What is this PR

  • [ ] Bug fix
  • [x] Addition of a new feature
  • [ ] Other

Why is this PR needed? This PR closes the issue #326. This PR adds support for common data imputation methods (bfill, ffill, nearest, and constant) to the interpolate_over_time function.

What does this PR do?

  • Extends the interpolate_over_time function to support bfill, ffill, nearest, and constant imputation methods.
  • Includes a fill_value parameter for the constant method.
  • Update the docstring of interpolate_over_time function for the relevant changes.

References

Issue #326

How has this PR been tested?

This PR has been tested locally by runnig the pytest and it pass all test cases. However, test cases specifically for this PR is not covered.

Is this a breaking change?

No

Does this PR require an update to the documentation?

Yes. I have update the docstring of the interpolate_over_time function accordingly.

Checklist:

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

angkul07 avatar Mar 18 '25 16:03 angkul07

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (bdb9fc6) to head (888e8e0). Report is 12 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #500      +/-   ##
===========================================
+ Coverage   99.86%   100.00%   +0.13%     
===========================================
  Files          28        28              
  Lines        1523      1563      +40     
===========================================
+ Hits         1521      1563      +42     
+ Misses          2         0       -2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Mar 18 '25 17:03 codecov[bot]

In the 890d1b2 commit, I removed the constant interpolate method because the fill_value parameter required by the constant method is mismatching with the xarray's interpolate_na function fill_value. So, it is failing some test cases. The futher discussion with the maintainer is required.

angkul07 avatar Mar 18 '25 17:03 angkul07

Re the point below:

I removed the constant interpolate method because the fill_value parameter required by the constant method is mismatching with the xarray's interpolate_na function fill_value.

Can we use interpolate_na instead then?

sfmig avatar Mar 25 '25 12:03 sfmig

Hello, @sfmig, first of all I would like to say sorry for my delayed response. I was busy in my gsoc proposal and some deadline.

I added some comments in line, but the most important feedback point is the need to add tests to this PR. Note that all code contributions should be properly tested, otherwise we cannot make sure our code behaves as expected.

This PR also aims to support bfill, ffill and constant value filling, but the docstring specifies a longer list of interpolation methods. If we support all these, they should all be tested. Otherwise they should be removed.

Thanks for pointing out. My goal was to add the test cases eventually. I first wanted to check the PR and wanted to get it reviewed. But as you suggest, I will add the test cases. The xarray interpolate method supports all the methods I mentioned in the docstring. So, I will add test cases for all these methods.

Thanks.

angkul07 avatar Apr 09 '25 15:04 angkul07

Re the point below:

I removed the constant interpolate method because the fill_value parameter required by the constant method is mismatching with the xarray's interpolate_na function fill_value.

Can we use interpolate_na instead then?

I will look at it and try to see if we can use it or not. I will report back to you.

angkul07 avatar Apr 09 '25 15:04 angkul07

@sfmig, I have added the test cases and add the support for all the interpolation methods(that were mentioned in the docstring). Also, I add the constant interpolation method too. Your feedback will be helpful. Thanks.

angkul07 avatar Apr 09 '25 17:04 angkul07

tests in the test_integration/test_filtering.py are failing. I believe that tests are failing because the fill_value(I used for constant method) in the interpolate_over_time function is mismatching with test_nan_propagation_through_filters function in test_integration/test_filtering.py. I need to check it further tho.

angkul07 avatar Apr 10 '25 23:04 angkul07

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

sonarqubecloud[bot] avatar Apr 16 '25 10:04 sonarqubecloud[bot]