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

Cleanup documentation generation for pyplot #22669

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Mar 19, 2022

  • remove the awkward pyplot.plotting() function, which only served as a namespace to take up the docs for pyplot and output them via .. autofunction
  • Instead generate the same information using .. autosummary::. We have to list the desired methods here explicitly. I've added a test that these are the same as previously auto-generated in the plotting() docstring. If we change anything in pyplot, we'll be notified through the test failure that we have to adapt the autosummary list.
  • Removed the docstring generation logic _setup_pyplot_info_docstrings(). Apart from generating the plotting() docstring, this added docstrings to the pyplot colormap setters. Instead, we now add these docstrings directly via boilerplate.py.

@timhoffm timhoffm added this to the v3.6.0 milestone Mar 19, 2022
@timhoffm timhoffm marked this pull request as draft March 19, 2022 10:04
@timhoffm timhoffm marked this pull request as ready for review March 22, 2022 18:36
@QuLogic
Copy link
Member

QuLogic commented Mar 22, 2022

  • We have to list the desired methods here explicitly.

Can it be done with automodule or similar, and the few exclusions from get_plot_commands?

@timhoffm
Copy link
Member Author

timhoffm commented Mar 23, 2022

Can it be done with automodule or similar, and the few exclusions from get_plot_commands?

It's actually not that easy because of the colormap setters.

I don't claim this to be the ultimate solution for the pyplot docs. It's only a more sane way of achieving similar docs to the existing ones. I'm even inclined to go more in the direction of the Axes docs where we have topic groups of functions, but that likely results in a refinement of the "test-everything-is-covered" approach here.

lib/matplotlib/tests/test_pyplot.py Outdated Show resolved Hide resolved
def test_doc_pyplot_summary():
"""Test that pypot_summary lists all the plot functions."""
pyplot_docs = Path(__file__).parent / '../../../doc/api/pyplot_summary.rst'
assert pyplot_docs.exists() # TODO: maybe skip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely needs a skip, since tests can be run while installed, without docs anywhere near this spot. I think we do something similar for pyplot.py.

lib/matplotlib/tests/test_pyplot.py Outdated Show resolved Hide resolved
lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved
lines = pyplot_docs.read_text()
m = re.search(r':nosignatures:\n\n(.*?)\n\n', lines, re.DOTALL)
functions = [line.strip() for line in m.group(1).split('\n')]
assert functions == plt.get_plot_commands()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is going to be pretty mysterious for most folks when they add a pyplot method to axes, and it causes this to fail. maybe a more verbose failure message? It seems if we make whole function docstrings with boilerplate, we could also make the list of functions in pyplot_summary as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that most folks will not be adding pyplot functions. That interface should not grow except for very rare cases such as stairs(), but sure I will add a more instructive error message.

It seems if we make whole function docstrings with boilerplate, we could also make the list of functions in pyplot_summary as well?

We could along with boilerplate. However, that list is very stable, changes probably less than once a year. I've saved the effort to write a complete generator. Only checking that it's up-to-date is much simpler. And similar to boilerplate for the code we anyway have to check whether it's updated. If not up-to-date, the fix is very easy. Telling the user: "add your_func to the list of plotting commands in pyplot_summary.rst" is the same effort and complexity for the user as "run this boilerplate script".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, so maybe just a more verbose message here.... It does mean people have to change two things, but agree that the list should rarely change so its not an undue burden...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a more instructive message.

@timhoffm timhoffm force-pushed the doc-pyplot branch 3 times, most recently from 80d2f62 to e411470 Compare May 29, 2022 21:01
- remove the awkward `pyplot.plotting()` function, which only served
  as a namespace to take up the docs for pyplot and output them via
  `.. autofunction`
- Instead generate the same information using `.. autosummary::`. We
  have to list the desired methods here explicitly. I've added a test
  that these are the same as previously auto-generated in the
  `plotting()` docstring. If we change anything in pyplot, we'll be
  notified through the test failure that we have to adapt the
  autosummary list.
- Removed the docstring generation logic
  `_setup_pyplot_info_docstrings()`. Apart from generating the
  `plotting()` docstring, this added docstrings to the pyplot colormap
  setters. Instead, we now add these docstrings directly via
  boilerplate.py

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@jklymak jklymak merged commit 8788dcc into matplotlib:main Jun 2, 2022
@timhoffm timhoffm deleted the doc-pyplot branch June 2, 2022 18:15
@QuLogic QuLogic mentioned this pull request Dec 6, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants