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

Add interpolators based on scipy's RegularGridInterpolator #44

Merged
merged 6 commits into from
Jan 26, 2023

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Jan 25, 2023

This PR adds interpolators based on scipy's RegularGridInterpolator.
One of the motivations is that the RegularGridInterpolator seems to be prefered to the RectBivariateSpline interpolator according to the interp2d migration instructions, as it does not depend on fitpack.
Performancewise, I could not see any significant difference, with maybe the new interpolator being slightly faster.

Another reason for adding this interpolator is that it has builtin support for extrapolation, although I haven't tested this extensively.

Lastly, the new interpolators have a different API for interpolation. Where we were providing the result grid already at instanciation time for the legacy interpolator, the new ones will only take this information when the interpolation method is called. This allows for more flexible interpolation, in particular we can build a lazy interpolation routine that work chunkwise with dask (supported in the proposed implementation).

  • Tests added
  • Tests passed
  • Passes git diff origin/main **/*py | flake8 --diff
  • Fully documented

@mraspaud mraspaud self-assigned this Jan 25, 2023
@mraspaud mraspaud changed the title Add interpolators base on scipy's RegularGridInterpolator Add interpolators based on scipy's RegularGridInterpolator Jan 25, 2023
@coveralls
Copy link

coveralls commented Jan 25, 2023

Coverage Status

Coverage: 87.186% (+1.2%) from 86.033% when pulling ea2f3c8 on mraspaud:add_grid_interpolator into 751dd62 on pytroll:main.

Comment on lines 272 to 276
Args:
method: the method to use for interpolation as described in RegularGridInterpolator's documentation.
Default is "linear".
chunks: If not None, a lazy (dask-based) interpolation will be performed using the chunk sizes specified.
The result will be a dask array in this case. Defaults to None.
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be missing fine_points.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed

@djhoese
Copy link
Member

djhoese commented Jan 25, 2023

This all looks good to me, but I'm not sure I should be the one to really judge it as I don't think I use the existing interpolators. Should the old interpolators be deprecated or changed to use the new classes underneath?

@mraspaud
Copy link
Member Author

This all looks good to me, but I'm not sure I should be the one to really judge it as I don't think I use the existing interpolators. Should the old interpolators be deprecated or changed to use the new classes underneath?

We could deprecate them, however we should look at the extrapolation feature of the new interpolators first to make sure they are good enough for us.

@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #44 (ea2f3c8) into main (751dd62) will increase coverage by 1.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
+ Coverage   86.90%   88.03%   +1.13%     
==========================================
  Files          21       21              
  Lines        1313     1438     +125     
==========================================
+ Hits         1141     1266     +125     
  Misses        172      172              
Flag Coverage Δ
unittests 88.03% <100.00%> (+1.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
geotiepoints/geointerpolator.py 94.23% <100.00%> (+2.56%) ⬆️
geotiepoints/interpolator.py 84.17% <100.00%> (+5.72%) ⬆️
geotiepoints/tests/test_geointerpolator.py 100.00% <100.00%> (ø)
geotiepoints/tests/test_interpolator.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mraspaud mraspaud merged commit 412f742 into pytroll:main Jan 26, 2023
@mraspaud mraspaud deleted the add_grid_interpolator branch January 26, 2023 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants