Skip to content

Question on docstring and signature of Axes.stem() #10151

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

Closed
timhoffm opened this issue Jan 3, 2018 · 7 comments
Closed

Question on docstring and signature of Axes.stem() #10151

timhoffm opened this issue Jan 3, 2018 · 7 comments

Comments

@timhoffm
Copy link
Member

timhoffm commented Jan 3, 2018

While working on #10148 I've come across https://matplotlib.org/api/_as_gen/matplotlib.axes.Axes.stem.html

I have the following questions.

1. Link to MATLAB

"See also" links to the MATLAB documentation. Do we really do this and is it meaningful? IMO the docs should be self-contained. There is no guarantee that the matplotlib and MATLAB APIs are or will stay compatible. I propose to simply remove the link.

2. keyword args

The signature is stem(*args, **kwargs). But it does only consider a limited list of 5 kwargs:

  • linefmt
  • markerfmt
  • basefmt
  • bottom
  • label

Currently, all other arguments are silently ignored.

I propose to explicitly write the allowed arguments in the signature:

stem(*args, linefmt=None, markerfmt=None, basefmt=None, bottom=0, label=None)

AFAICS this should still be fully API compatible even with all variants of an injected data keyword and the possibility to specify the formats as positional arguments.

The question is how to handle other kwargs. Options are:

a) still silently ignore them (no change)
b) raise a TypeError (breaks API compatibility, but would be the standard sane python behavior)
c) issue a warning

I tend to b) in this case.

@anntzer
Copy link
Contributor

anntzer commented Jan 3, 2018

The actual implementation needs to use kwargs as long as we maintain Py2 compat (because otherwise the arguments become positional-or-keyword, which doesn't make much sense). The doc can write the "actual" signature in the first line (http://www.sphinx-doc.org/en/stable/ext/autodoc.html#confval-autodoc_docstring_signature).

Per standard policy I would throw a deprecation warning in 2.2 for unhandled arguments. This way, the next release (which should hopefully be 3 and Py3 only...) can just switch to using normal kw-only arguments.

@jklymak
Copy link
Member

jklymak commented Jan 3, 2018

I think its OK-ish for "See Also" to link to other documents or packages so long as the docs don't require that See Also... I think of it as a "reference" section.

See #4829 for the meaning of the @_preprocess_data decorator, though I admit I still don't understand it. But I think it requires the args in that form?

@timhoffm
Copy link
Member Author

timhoffm commented Jan 4, 2018

@anntzer the positional-or-keyword issue is already present in the current implementation (stem(y, linefmt='g') vs. stem(y, 'g')) and is explicitly handled in the code. This wouldn't change with explicit keywords.

@jklymak @_preprocess_data can also work with explicit args and kwargs. See e.g. Axes.pie

For now, I've resorted to auto_docstring_signature. But that ignores @_preprocess_data, which I have to add manually. I feel that adding more magic is the wrong direction.

@anntzer
Copy link
Contributor

anntzer commented Jan 4, 2018

Ah, I see. Unless there's a (likely...) dragon in the rabbit hole, I'm +1 to just move the kwargs into the signature then.

@timhoffm
Copy link
Member Author

timhoffm commented Jan 4, 2018

Note: auto_docstring_signature is not supported/filtered out in pyplot._setup_pyplot_info_docstrings(). I.e. the signature would be part of the overview in the stem description on https://matplotlib.org/api/pyplot_summary.html.

If we really need this mechanism some time, pyplot._setup_pyplot_info_docstrings() must obtain the capability to strip the signature from the docstring.

For the present case, I will revert the docstring signature and try to pursue explicit keywords.

@timhoffm
Copy link
Member Author

timhoffm commented Jan 4, 2018

Ok, found the dragon in the rabbit hole: The issue is having a variable number of positional-only arguments (y or x, y) and some additional arguments that can be positional or keyword (the *fmt args).

In particular, simultaneously satisfying the following call patterns is not possible with explicit signatures in Python 2:

stem(y, linefmt='--')
stem(y, '--')
stem(x, y)

Python 3 would be fine with

stem(*args, linefmt=None, markerfmt=None, basefmt=None, bottom=0, label=None)

This makes the *fmts keyword only. The internal logic in the mehod can check if they are set or not, and try to obtain them from *args otherwise.

Python 2 requires *args to be behind explicit keywords:

stem(linefmt=None, markerfmt=None, basefmt=None, bottom=0, label=None, *args)

Then, I cannot stem(y, linefmt='--') because Python would try to bind both arguments to linefmt.

I'm reverting to the generic signature, since we currently cannot do better without breaking the API.

@timhoffm
Copy link
Member Author

Closing as "good enough".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants