-
Notifications
You must be signed in to change notification settings - Fork 54
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
Refactor: Replace interp2d with interpn #279
Refactor: Replace interp2d with interpn #279
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.5.0 #279 +/- ##
=============================================
- Coverage 47.41% 47.21% -0.21%
=============================================
Files 14 14
Lines 2708 2728 +20
=============================================
+ Hits 1284 1288 +4
- Misses 1424 1440 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall @lettlini and does improve readability.
I realized that my first comment is a general question, why did you swap the coordinates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving now after the small fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy for this to be merged. I think we can ignore the failed codecov
test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this with a fairly large dataset and got the same answers as with the original. Happy for this to be merged. Thanks, @lettlini .
@lettlini are you happy for this to be merged? If so, I'll go ahead and get it in. One step closer to v1.5 release. |
Thanks for the reviews @snilsn @w-k-jones @freemansw1. Happy for this to be merged. |
This PR replaces all calls to
interp2d
withinterpn
.When conducting a review, unfortunately, caution is required in some places, as not all situations are covered by unit tests.