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

Improve and fix *Spline docstrings and code #11756

Closed
erheron opened this issue Mar 30, 2020 · 1 comment · Fixed by #12463
Closed

Improve and fix *Spline docstrings and code #11756

erheron opened this issue Mar 30, 2020 · 1 comment · Fixed by #12463
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.interpolate
Milestone

Comments

@erheron
Copy link
Contributor

erheron commented Mar 30, 2020

During the investigation of the issue #9718 I've discovered a few possibilities for both improvements and fixes in the documentation of different *Spline classes in
https://github.com/scipy/scipy/blob/master/scipy/interpolate/fitpack2.py
I create this issue mainly because it seems to me that it'll be (and must be) more general than #9718
The things below will be mostly about RectBivariateSpline class

Things that came into mind:

  • In BivariateSpline class, as far as I understand, RectBivariateSpline should be added to the end of the sentence, because it subclasses BivariateSpline
    This class is meant to be subclassed, not instantiated directly.
    To construct these splines, call either `SmoothBivariateSpline` or
    `LSQBivariateSpline`.
  • Docstring of RectBivariateSpline containes a few more inconsistencies: e.g. tx, ty in here:
    bbox : array_like, optional
    Sequence of length 4 specifying the boundary of the rectangular
    approximation domain. By default,
    ``bbox=[min(x,tx),max(x,tx), min(y,ty),max(y,ty)]``.

    although they do not appear in init:
    def __init__(self, x, y, z, bbox=[None] * 4, kx=3, ky=3, s=0):
  • I have a feeling that "See Also" sections in different Spline's might be more... broad? Consistent? There's not so many of that classes, so why not to mention them all, or at least ensure that if A -> B and A -> C (-> means subclassing), then B mentions C in "See Also" and vice versa?
  • I've got a feeling that two last checks of these 4 are redundant, but I'm not sure
    if not np.all(diff(x) > 0.0):
    raise ValueError('x must be strictly increasing')
    if not np.all(diff(y) > 0.0):
    raise ValueError('y must be strictly increasing')
    if not ((x.min() == x[0]) and (x.max() == x[-1])):
    raise ValueError('x must be strictly ascending')
    if not ((y.min() == y[0]) and (y.max() == y[-1])):
    raise ValueError('y must be strictly ascending')
  • Problem with DOC: docstring inconsistency in RectBivariateSpline #9718 remains for now, but I'm working on it

If this issue "makes sense" to some of the members, I suggest closing #9718 in favor of this one.

@miladsade96 miladsade96 added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.interpolate labels Mar 30, 2020
@stefanv
Copy link
Member

stefanv commented May 3, 2020

Thank you, @erheron, a thorough review of this and related docs would be great. All the inconsistencies you noted should definitely be addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.interpolate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants