-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
MAINT: Harmonized documentation for Interpolator classes #18413
Conversation
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, several suggestions below.
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 now, one optional suggestion below.
If you want to address it, please do; otherwise we can merge this PR as is.
axis : int, optional | ||
Axis in the y array corresponding to the x-coordinate values. | ||
Axis in the ``y`` array corresponding to the x-coordinate values. Defaults | ||
to ``axis=0``. |
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.
So you wanted to stress that by default it's the first axis; do you want try keeping it two ways? Defaults to the first axis, axis=0
or some such?
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.
Instead, I stressed that unlike other interpolators, the interpolation axis in interp1d
is the last one. So as far as I'm concerned I'm happy with this sentence
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 now, one optional suggestion below.
If you want to address it, please do; otherwise we can merge this PR as is.
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 now, one optional suggestion below.
If you want to address it, please do; otherwise we can merge this PR as is.
Thank you @gjhuizing and congartulations with what I believe is your first scipy commit. Keep them coming! |
Thanks a lot for the review! |
Reference issue
#18165 (comment)
What does this implement/fix?
In the documentation, I harmonized and clarified the dimensions of the inputs of *Interpolator objects. I chose the
(npoints, ndims)
convention. In addition, since the legacy classinterp1d
uses the last axis and other interpolators use the first axis by default, I emphasized the default value in the docstring.Additional information
This is a modest change but I'm hoping to motivate more ambitious harmonization of the interpolation module. In particular, naming the inputs
points
andvalues
in all interpolators would make it easier for the user (see my original issue). In addition, some interpolators allow specifying the interpolation axis while others don't, which prevents the user from seamlessly interchanging one interpolator for another.Best,
GJ