Refactor IDW interpolation: simplify logic and clarify API#877
Open
ecomodeller wants to merge 7 commits into
Open
Refactor IDW interpolation: simplify logic and clarify API#877ecomodeller wants to merge 7 commits into
ecomodeller wants to merge 7 commits into
Conversation
- Make get_idw_interpolant private (_get_idw_interpolant) - Remove unnecessary 1D/2D conditional logic - Require 2D input (callers use np.atleast_2d if needed) - Add proper documentation with assumptions (sorted distances) - Remove direct unit test (covered by integration tests) - Update scatter interpolation examples with copy-pasteable implementation - Remove reliance on private functions in documentation and notebooks This follows Sandi Metz principles: - Reduce conditional complexity - Test public interfaces, not implementation details - Clear public vs private API separation
Use np.zeros_like and np.ones_like instead of np.zeros/np.ones to preserve input dtype (float32) throughout interpolation. DFS files use float32 to save memory, and the previous implementation was converting to float64, doubling memory usage during Dataset.interp_like operations. Changes: - _interpolation.py: np.zeros(distances.shape) → np.zeros_like(distances) - _FM_geometry.py: np.ones(dists.shape) → np.ones_like(dists) - Add tests to verify float32 dtype preservation
ryan-kipawa
requested changes
Jan 30, 2026
Contributor
ryan-kipawa
left a comment
ryan-kipawa
left a comment
There was a problem hiding this comment.
I agree with the overall principles of this PR. However, I suspect this is very common functionality, and since it's been in the documentation for so long, it could be argued it's already part of the public API despite having a leading underscore. How do you see that?
We could consider
- Support such functionality in the public API
- If we remove it, keep _get_idw_interpolate with a deprecation warning that it will be removed and is not part of the public API
Member
Author
|
I was made aware of yet another use case for interpolating scatter data onto a mesh to create a dfsu. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why this matters
Documentation that imports private functions teaches users to depend on implementation details. When users copy examples from docs, those imports become de facto public API—making internal refactoring a breaking change. This undermines the purpose of private/public separation and makes version upgrades break user code.
Changes
Code simplification
get_idw_interpolant→_get_idw_interpolant(make private)keepdims=TrueTesting
test_interp2d_same_pointsverifies exact match handlingDocumentation
simple_idw()function