Skip to content

Add reference section to all statistics examples #19231

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

Merged
merged 2 commits into from
Jan 5, 2021

Conversation

ianhi
Copy link
Contributor

@ianhi ianhi commented Jan 4, 2021

PR Summary

As the title says.

I didn't add every function used in the examples, rather I erred on the side of caution against overlinking things such as set_ylabel and tried to include only the functions that the examples focus on.

PR Checklist

  • [NA] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [NA] New features are documented, with examples if plot related.
  • [NA] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [NA] Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [NA] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [NA] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@ianhi
Copy link
Contributor Author

ianhi commented Jan 4, 2021

Three other thoughts:

  1. There are a lot of boxplot examples
  2. bxp and boxplot never have their difference coherently explained as far as I can tell
  3. Are PRs like this one desireable? Better linking in the docs seems like a positive, but I don't want to add uncesssary noise

@jklymak
Copy link
Member

jklymak commented Jan 4, 2021

This is very welcome. We just haven't gotten around to doing every example yet and have been counting on people just adding them as needed.

@timhoffm
Copy link
Member

timhoffm commented Jan 4, 2021

I didn't add every function used in the examples, rather I erred on the side of caution against overlinking things such as set_ylabel and tried to include only the functions that the examples focus on.

👍

  1. There are a lot of boxplot examples

There's quite some redundancy in some areas of the examples; I guess for historic reasons. More and more examples were added and it was not always checked if this adds something new or if there should have been some rearrangement. A bit consolidation would be good. But that's also a tedious process. One has to find out what the core message of each example is and if that's already explained in another example. If you see something that is duplicate or if some examples could be merged easily, you're welcome to open a PR.

  1. Are PRs like this one desireable? Better linking in the docs seems like a positive, but I don't want to add uncesssary noise

Yes definitively. The references are value in particular because of the backlinks from the functions to the examples. But choosing the relevant references must be done manually.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Generally, we add the corresponding pyplot functions as well, even if they are not used. See e.g. https://51381-1385122-gh.circle-artifacts.com/0/doc/build/html/gallery/statistics/barchart_demo.html#sphx-glr-gallery-statistics-barchart-demo-py

This is quite a crutch, but the only way, that the pyplot functions get to show the examples as well.

# The use of the following functions and methods is shown in this example:

import matplotlib
matplotlib.transforms.Affine2D
Copy link
Member

Choose a reason for hiding this comment

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

I'd add matplotlib.patches.Ellipse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. Although it seems that the backlinking does not work for classes like Ellipse or Affine2D. See https://51381-1385122-gh.circle-artifacts.com/0/doc/build/html/api/transformations.html#matplotlib.transforms.Affine2D where this example is not linked to :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added that the Ellipse in the latest push, though the issue with it rendering will remain I suppose

@ianhi
Copy link
Contributor Author

ianhi commented Jan 4, 2021

Generally, we add the corresponding pyplot functions as well, even if they are not used

Ideally this could be automated to catch both pyplot -> axes and axes -> pyplot?

@ianhi
Copy link
Contributor Author

ianhi commented Jan 4, 2021

Generally, we add the corresponding pyplot functions as well

Done

@timhoffm
Copy link
Member

timhoffm commented Jan 4, 2021

Generally, we add the corresponding pyplot functions as well, even if they are not used

Ideally this could be automated to catch both pyplot -> axes and axes -> pyplot?

Unfortunately this is all very manual: We're tuning our content so that sphinx-gallery does "the right thing". But sphinx-gallery does not know anything about pyplot <-> axes relations. And we cannot easily patch this in.

Probably the most promising direction of automation would be to customize the generation of the pyplot ReST code, so that it references the respective Axes minigallery; i.e.

matplotlib.pyplot.boxplot
=========================

.. currentmodule:: matplotlib.pyplot

.. autofunction:: boxplot

.. minigallery:: matplotlib.axes.Axes.boxplot
   :add-heading:

instead of .. minigallery:: matplotlib.pyplot.boxplot.

That way:

  • we could limit ourselves to only giving Axes methods as references. (I see this as a plus, because in general we recommend the OO approach over pyplot.)
  • Pyplot functions would still have a helpful minigallery.
  • We don't need any customization in sphinx-gallery.

But up until somebody comes up with such a solution, we should go the manual route.

@QuLogic
Copy link
Member

QuLogic commented Jan 5, 2021

I think you can continue that discussion in #19232.

@QuLogic QuLogic merged commit a5ea869 into matplotlib:master Jan 5, 2021
@QuLogic QuLogic added this to the v3.4.0 milestone Jan 5, 2021
@ianhi ianhi deleted the demos branch January 5, 2021 02:01
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.

4 participants