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

signature in accessor methods #4152

Closed
keewis opened this issue Jun 13, 2020 · 5 comments · Fixed by #4323 or #4359
Closed

signature in accessor methods #4152

keewis opened this issue Jun 13, 2020 · 5 comments · Fixed by #4323 or #4359

Comments

@keewis
Copy link
Collaborator

keewis commented Jun 13, 2020

With the merge of #3988 we're now properly documenting the str, dt and plot accessors, but the signatures of the plotting methods are missing a few parameters. For example, DataArray.plot.contour is missing the parameter x (it's properly documented in the parameters section, though).

Also, we need to remove the str and dt accessors from Computing and try to figure out how to fix the summary of DataArray.plot (the plotting method).

@keewis
Copy link
Collaborator Author

keewis commented Jul 2, 2020

it seems fixing both the signature and the summary is a bit more involved than I thought and probably either requires changes to sphinx or partially rewriting the Autosummary class. Since using sphinx-autosummary-accessors will probably be the way to go here, let's continue that discussion in keewis/sphinx-autosummary-accessors#6

@keewis
Copy link
Collaborator Author

keewis commented Jul 3, 2020

I hacked around that issue in the PR so this issue would be fixed by switching to sphinx-autosummary-accessors. However, since RTD pins us to sphinx=1.8.5 (we can't change that when using conda) and the way the hack is written right now requires sphinx>=3.1, we won't be able to use it.

@keewis
Copy link
Collaborator Author

keewis commented Jul 4, 2020

actually, it seems RTD unpinned the dependency versions (not sure when they did that) so we might even be able to make use of the hack. So: should we try using that package by installing from github, or would it be better to wait until there's a released version available?

@keewis
Copy link
Collaborator Author

keewis commented Aug 10, 2020

unfortunately, sphinx-autosummary-accessors can't fix all of this: some of the plotting functions have a signature of (x, y, z, ax, **kwargs). After passing through the autosummary / autodoc, this becomes (y, z, ax, **kwargs) (x was assumed to be self or darray). However, this doesn't match the documentation which claims to have darray, x, y and ax instead. Maybe we should try to fix that?

@keewis keewis reopened this Aug 10, 2020
@keewis
Copy link
Collaborator Author

keewis commented Aug 11, 2020

the issue is that we use functools.wraps to copy __doc__, __name__ and __qualname__ to newplotfunc. How about using

    # Build on the original docstring
    dummy.__doc__ = f"{plotfunc.__doc__}\n{commondoc}"

    # to shorten the signature
    @functools.wraps(plotfunc)
    def dummy(darray, x, y, **kwargs):
         pass

    # use the signature of dummy
    del dummy.__wrapped__

    @functools.wraps(dummy)
    def newplotfunc(

instead of

xarray/xarray/plot/plot.py

Lines 572 to 576 in 1791c3b

# Build on the original docstring
plotfunc.__doc__ = f"{plotfunc.__doc__}\n{commondoc}"
@functools.wraps(plotfunc)
def newplotfunc(

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 a pull request may close this issue.

1 participant