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

Define consistent nodata propagation for interp_points and allow to return interpolator #560

Merged
merged 29 commits into from
Sep 3, 2024

Conversation

rhugonnet
Copy link
Contributor

@rhugonnet rhugonnet commented Jun 7, 2024

Most of the line changes in this PR are due to re-structuring, moving out functions of the Raster class (see #539, in preparation for #383) into their own module, here only those linked to interp_points.

This PR also adds a nodata propagation scheme (as scipy.interpolate.interpn does not support nodata propagation natively except for methods "nearest" and "linear") and allows to return an interpolator that contains this nodata propagation (to optimize speed of iterative methods calling the same interpolator several times, such as DEM coregistration).

It turned out that interpolation of map_coordinates with splines of order 3/5 does not match "cubic" or "quintic" in interpn, only order 0/1 do match for "nearest" and "linear", so all other are dropped. The nodata propagation and accuracy of results for values near the edge of NaNs is now tested thoroughly! 😄

Resolves #559
Starts addressing #539 for functions related to interp_points

TO-DO:

  • Add tests on return_interpolator,
  • Add tests validating that the interpolator generated produces exactly the same results as interpn,
  • Add distance of nodata spreading as an argument,
  • Enforce the same NaN propagation between map_coordinates and interpn.

@rhugonnet rhugonnet changed the title Add option to return interpolator in interp_points and re-structure Define consistent nodata propagation for interp_points and allow to return interpolator Jun 8, 2024
@rhugonnet
Copy link
Contributor Author

rhugonnet commented Aug 6, 2024

This PR is finalized, and ready for review! 😄 @adehecq @erikmannerfelt @atedstone
Important: don't lose time looking at changes from functions moved from raster.py to georeferencing.py, etc... This is simply restructuring linked to #539, which was necessary to have functionalities separate from the Raster class (to avoid always re-building a Raster object for performance, for instance in coregistration), and will facilitate building the Xarray accessor as well.
All the core changes are in interpolate.py and test_interpolate.py.

We now have a fully consistent point interpolation with all benefits and (normally) none of the limitations of existing implementations:

  • Very fast on an equal grid (by relying on scipy.ndimage.map_coordinates) which supports "nearest" or "linear" methods,
  • Otherwise (for a regular grid, or other interpolation methods), it relies on the equivalent of scipy.interpolate.interpn but coded as an interpolator function based on SciPy's code (so that it can be constructed only once and used iteratively, to greatly improve performance for instance in coregistration),
  • Supports modular NaN propagation for all methods by adding our own consistent NaN propagation scheme on top of SciPy (that barely supports any NaN propagation in interpolation, only "nearest" and "linear" do in interpn and inconsistently with map_coordinates), with our default method tested for its high accuracy (propagate NaNs for as many pixels as half the method order rounded up: 1 for linear, 2 for cubic, 3 for quintic).

All the tests allow to check the robustness of the implementation:

  • Exact same results for interpn and map_coordinates, and the interpolator optionally returned,
  • Exact same NaN propagation,
  • High accuracy of interpolation near NaN edges (10e-4 relative) for the default NaN propagation.

Copy link
Member

@adehecq adehecq left a comment

Choose a reason for hiding this comment

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

I have a few comments, but otherwise looking good to me!
Thanks! 🙌

@rhugonnet rhugonnet merged commit 5d2a856 into GlacioHack:main Sep 3, 2024
22 checks passed
@rhugonnet rhugonnet deleted the return_interpolator branch September 3, 2024 23:30
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.

Add option to return interpolator of interp_points to optimize speed of some iterative functionalities
2 participants