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

DOC: new plot gallery #19703

Merged
merged 11 commits into from
Apr 5, 2021
Merged

DOC: new plot gallery #19703

merged 11 commits into from
Apr 5, 2021

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Mar 14, 2021

PR Summary

Start on a new simple plot gallery just for plot types. Heavily based on the cheatsheet @rougier, and suggested by @story645

Closes #18277

PR Checklist

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

@jklymak
Copy link
Member Author

jklymak commented Mar 14, 2021

This needs a bit off work to pass but here is the general idea:

https://54955-1385122-gh.circle-artifacts.com/0/doc/build/html/
https://54955-1385122-gh.circle-artifacts.com/0/doc/build/html/plot_types/index.html

Click on Plot Types

@story645
Copy link
Member

My bias is to organize by mark type/base artist - marker, line2d, patch...but that might confuse folk...I'm really liking it on a first pass, but is the plan to keep all the cheatsheat code or simplify to just the data/plot?

@timhoffm
Copy link
Member

Random thought: A flow-chart approach to organization could also be nice e.g. something like https://twitter.com/jamiejgriffin/status/1179758518427107328

but I don't think one can easily create that.

@jklymak
Copy link
Member Author

jklymak commented Mar 15, 2021

So, I don't think we should workshop this to death yet.

If we think a third gallery like this is a good idea, then let us add it. We can then discuss how its styled, exactly what is plotted, how it is organized etc after, preferably as PRs on the already existent gallery.

FWIW, I think copying the styling from the cheatsheet is a good strategy to keep the overviews parallel. These all should have links added to them over the next little while, but that is pretty good First-Issue fodder if the scaffolding is in place.

@jklymak jklymak marked this pull request as ready for review March 15, 2021 01:37
@jklymak jklymak added this to the v3.5.0 milestone Mar 15, 2021
@timhoffm
Copy link
Member

I like the cheat-sheet style. However, the styling heavily clutters the actual command.

Can we make https://54953-1385122-gh.circle-artifacts.com/0/doc/build/html/plot_types/A_basic/a_plot.html#sphx-glr-plot-types-a-basic-a-plot-py something like

import matplotlib.pyplot as plt
import numpy as np
plt.style._use_cheatsheet_style()  # style only used for the examples

X = np.linspace(0, 10, 100)
Y = 4 + 2 * np.sin(2 * X)

fig, ax = plt.subplots()
ax.plot(X, Y)

plt.show()

Then again, while the defaults are not as pretty, it would make some sense to show the plots like the users would see them without heavy styling.

@jklymak
Copy link
Member Author

jklymak commented Mar 15, 2021

The biggest issue isn't particularly the defaults but the labels - they detract from the bare images, hence the manual axes placement. I tried to make the preamble and postamble as similar as possible so they would be easy to change...

@timhoffm
Copy link
Member

timhoffm commented Mar 16, 2021

We can't remove the labels using styles, can we?

  1. Oh, maybe make them white/transparent?

  2. Alternatively,

    import matplotlib.pyplot as plt
    import numpy as np
    
    with plt.style._use_cheatsheet_style():  # style only used for the examples
    
        X = np.linspace(0, 10, 100)
        Y = 4 + 2 * np.sin(2 * X)
    
        fig, ax = plt.subplots()
        ax.plot(X, Y)
    
  3. Or we don't separate figure code and displayed code by using .. plot: and .. code-block::.
    At the slight annoyance that code and figure may not be in sync. But anyway the figure is only an illustration.
    And I anticipate virtually no changes to the basic API which we need there. So duplication is less critical.

  4. Or we need heavier sphinx machinery with a custom directive.

Thinking of it, 3. seems the reasonable choice.

@story645
Copy link
Member

Some version of 3, where the cheatsheet code is used to generate the figure + label for the gallery landing page, but the displayed figure + code on the actual plot page is the default styled bare minimum one? Which I think maybe is more suited to potentially growing the entry to all the configs?

@jklymak
Copy link
Member Author

jklymak commented Mar 16, 2021

I'm not convinced we should have some complicated solution here. The point of these in my mind is to give the user an idea of what is available and the name so they have. something to google or search the docs for. I don't feel some boilerplate styling around that info on the clicked page is all that distracting from the goal. I also don't know that demonstrating the default style is a particular goal.

Certainly we can add a style to help encapsulate, but I wouldn't let perfect get in the way of good here, particularly if it means hard-to-maintain extra workarounds.

@story645
Copy link
Member

My goal w/ this was

  • make explicit the structure of the data each function takes
  • illustrate the api of each function

and my experience has been that any extra line of code ends up being a place folks can trip up, especially since lots of users wholesale copy the whole example w/o really sorting out the parts they need.

@jklymak jklymak force-pushed the doc-new-gallery branch 3 times, most recently from e864aac to ce3e40e Compare March 16, 2021 17:05
@jklymak
Copy link
Member Author

jklymak commented Mar 16, 2021

Encapsulated the style changes in a style sheet. Happy to make private if necessary, but I'm not sure what the point of that would be.

I feel this is an improvement as-is, but if I were doing anything more to it, I'd include "see-also" blocks for each of them to act as an index to the gallery. But again, that can be done after the fact, and is easy for new contributors.

@jklymak jklymak force-pushed the doc-new-gallery branch 6 times, most recently from 435ac6f to 20fbf36 Compare March 16, 2021 19:24
@jklymak
Copy link
Member Author

jklymak commented Mar 16, 2021

axes.grid: True
axes.axisbelow: True # grid/ticks are below elements (e.g., lines, text)

xtick.color: 555555
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to set x/ytick rcs?
(also, quite a few other rcs seem to just match the default value?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried to minimize it.

Copy link
Member

Choose a reason for hiding this comment

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

Can't we have fully transparent color for the ticks ? On the gallery, I can see part of them (on the left here for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I've made their length 0

with plt.style.context('cheatsheet_gallery'):
fig, ax = plt.subplots()

ax.plot(X, Y, color="C1", linewidth=2.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for using C1 (orange)? just from the peanut gallery, C0 (blue) may perhaps look better, and it's the default too?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, so I tried changing the color order. Guess what - a bunch of artists do not respect the the rcParam when changed in the context manager. i.e. most of the stats ones.

WRT to orange, that is for consistency with the cheatsheet. You can argue with @rougier about what looks better ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, the issue with the colororder not sticking was if plt.show() was inside the context manager or outside. Some artists delay assigning the color until draw time. I think we should stomp out any delayed state in the library as it almost always makes things confusing....

Copy link
Member

Choose a reason for hiding this comment

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

I think "C0" might do, it mostly depends on the matplotlib website main color.

ax.plot(X, Y, color="C1", linewidth=2.0)

ax.set_xlim(0, 8), ax.set_xticks(np.arange(1, 8))
ax.set_ylim(0, 8), ax.set_yticks(np.arange(1, 8))
Copy link
Contributor

Choose a reason for hiding this comment

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

either ax.set(xlim=(0, 8), xticks=range(1, 8), ylim=(0, 0, yticks=range(1, 8)) or split over 4 lines or at least use a semicolon (a comma happens to work too, but is rather unidiomatic)

Copy link
Member Author

Choose a reason for hiding this comment

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

did carriage returns.

@anntzer
Copy link
Contributor

anntzer commented Mar 16, 2021

Some minor nits, but I like this very much.

@jklymak
Copy link
Member Author

jklymak commented Mar 17, 2021

@jklymak
Copy link
Member Author

jklymak commented Mar 17, 2021

Here is how this looks with no styling (except note I manually specified the colormap in imshow, and manually removed the ticks. Not the end of the world, and I could be convinced that simpler is better if that is the consensus:

boooooo

@rougier
Copy link
Member

rougier commented Mar 17, 2021

Aspect should be set to 1 and tick labels could be bigger.

@jklymak
Copy link
Member Author

jklymak commented Mar 17, 2021

Aspect should be set to 1 and tick labels could be bigger.

What is being proposed is linked here which is almost aspect=1 and has no labels: https://55085-1385122-gh.circle-artifacts.com/0/doc/build/html/plot_types/index.html

This snapshot above was because @story645 was suggesting no styling - so if we go that route, then we get what the default style gives us, or we add a bunch of bespoke code.

@timhoffm
Copy link
Member

@jklymak I really like what you are doing here! Thanks for taking this on.

I'm undecided concerning styling. The styled version clearly looks better, but has more distracting boilerplate code.

Possibly either one is fine for a start. We can always tune later.

@jklymak
Copy link
Member Author

jklymak commented Mar 17, 2021

Suggest we go with "looks good" and make simpler if there are complaints of confusion.

@@ -0,0 +1,19 @@
# from the Matplotlib cheatsheet as used in our gallery:
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to make this public then?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could? Whichever is easiest - I'm not sure about user examples that don't have a public stylesheet....

Copy link
Member

@timhoffm timhoffm Apr 2, 2021

Choose a reason for hiding this comment

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

IMHO it's ok to make the style public. If somebody wants to use that style, that's ok.

Is "cheatsheet_gallery" a good public name? I'm not sure if "cheatsheet" is a helpful term here, in particular also because the style looks a bit like the cheatsheets but is not used there. What about "mpl_plot_gallery"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@QuLogic QuLogic Apr 5, 2021

Choose a reason for hiding this comment

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

I guess it's okay to leave public, but I'm not sure we want to say that it will follow any API guarantees. It's tied very much with out documentation style, which may change at any time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, thats a good point. Happy to do a follow up to fix that...

@@ -0,0 +1,6 @@
.. _basic_plots:
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this in the top-level (of the repo, not the docs site) instead of in doc?

Copy link
Member Author

Choose a reason for hiding this comment

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

The other galleries are at the top level...

plot_types/A_basic/b_scatter.py Outdated Show resolved Hide resolved
plot_types/A_basic/c_bar.py Outdated Show resolved Hide resolved
# plot
fig, ax = plt.subplots()

ax.step(X, Y, linewidth=2.5)
Copy link
Member

Choose a reason for hiding this comment

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

Weird to have where= in the page title, but not shown.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a gallery, not documentation for the functions. So the title provides useful keywords the user may be interested in, but we aren't necessarily trying to demo those kwargs.

Copy link
Member

Choose a reason for hiding this comment

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

See my proposal above for leaving parameters out of the title and optionally add them to the body.

@@ -0,0 +1,31 @@
"""
====================
pie(X, explode, ...)
Copy link
Member

Choose a reason for hiding this comment

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

Again, explode is not shown.

Copy link
Member Author

Choose a reason for hiding this comment

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

... as above...

fig, ax = plt.subplots()

plt.contourf(X, Y, Z, levels=levs)
plt.contour(X, Y, Z, levels=levs, colors="white", linewidths=0.5)
Copy link
Member

Choose a reason for hiding this comment

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

At first, this looked like the bug with alpha and multiple filled contours to me. I don't know if you want to use a different colour for the contours, but I'm not sure which colour matches the theme.

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'm not following this concern.... what is wrong with just adding white contours on top of the contourf?

Copy link
Member

Choose a reason for hiding this comment

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

When I first saw it, I thought it was #4419 and not on purpose.

plot_types/C_stats/a_hist.py Outdated Show resolved Hide resolved
plot_types/C_stats/b_boxplot.py Outdated Show resolved Hide resolved
Comment on lines 1 to 5
"""
======================
plot([X], Y, [fmt]...)
======================
"""
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 leave out the parameters in the title. They are a bit cluttered.

We should however add a bit of info inside the example, e.g.

  • What this plot is. In the simplest case, this is just a repetition of the title line of the function documentation. But one might also give a little more context.
  • Prominent link to the docs
  • Optional: You could add the pseudo-signature from the title if you think that is helpful.
Suggested change
"""
======================
plot([X], Y, [fmt]...)
======================
"""
"""
======
plot()
======
"""
.. code-block:: none
plot([X], Y, [fmt]...)
Plot y versus x as lines and/or markers.
Documentation: `matplotlib.axes.Axes.plot()` / `matplotlib.pyplot.plot()`

Copy link
Member Author

Choose a reason for hiding this comment

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

This was basically copying the cheatsheet, which I think was pretty good. If you would like to simplify it later, and add more info to the main text, I think we could do as follow-ups.

@@ -0,0 +1,25 @@
"""
=====================
step(x, y, where=...)
Copy link
Member

Choose a reason for hiding this comment

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

We want to have stairs() as well. Two alternatives:

  • name this "step-like" and show both here
  • or make a separate entry and link between the two.

Copy link
Member Author

Choose a reason for hiding this comment

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

As above, we can add more, or modify as wanted after this is merged.

# plot
fig, ax = plt.subplots()

ax.step(X, Y, linewidth=2.5)
Copy link
Member

Choose a reason for hiding this comment

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

See my proposal above for leaving parameters out of the title and optionally add them to the body.

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.

Basically, good to go. As discussed there is still room for improvement, but that doesn't have to be part of this PR.

One last request from my side: I don't quite like the letter prefixes in the filenames. I assume that's for sorting? Can we configure that in gallery_order.py instead? Trying to sort by prefix will break if we want to change something later. We'd have to re-index, which breaks links (or needs redirects). Also the prefixes look ugly.

@jklymak
Copy link
Member Author

jklymak commented Apr 3, 2021

I agree if there is a reasonable way to order gallery entries. I wasn't aware one existed.

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.

This is a solid start. As discussed, there is still room for improvement, but let's make the changes incrementally and not let perfect be the enemy of good.

@timhoffm timhoffm merged commit 183018c into matplotlib:master Apr 5, 2021
@jklymak jklymak deleted the doc-new-gallery branch April 5, 2021 21:21
@jklymak jklymak added this to Done in Chart Types Gallery May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Create new sphinx gallery page for "Chart Types"
6 participants