Skip to content

Improve tutorial figures in the new theme #20546

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 8 commits into from
Jul 19, 2021

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Jun 29, 2021

PR Summary

Figures were originally 50% scale, but this looks tiny in the new theme. Mostly this undoes that, plus some tweaks in examples so that they don't end up too wide at 100%. For example, there's no need for this to be so small:

image

As done in other PRs, this also drops captions which seem pretty uninformative.

Finally, it makes some tweaks to some of the colour figures, by
a) moving colormaps to their respective tutorial sections

(preventing sphinx-gallery from treating them as a list and half-sizing them, like this) ![image](https://user-images.githubusercontent.com/302469/123762691-0aeb9180-d891-11eb-9b4c-2faa5d793e13.png)
b) and improving the xkcd vs CSS colour comparison chart by changing the text colour to contrast the background, plus some tweaks to make better alignment:

image

PR Checklist

  • [n/a] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [n/a] 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).
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@QuLogic QuLogic added this to the v3.5.0 milestone Jun 29, 2021
@QuLogic
Copy link
Member Author

QuLogic commented Jun 29, 2021

Of course, the effect of the example of the figure being too small is a bit stymied by GitHub resizing the figure to be as large as its description box.

@QuLogic
Copy link
Member Author

QuLogic commented Jun 29, 2021

It looks like the examples/shapes_and_collections/fancybox_demo.py change will conflict with #20538, but that's shaping it into a much better example, so I'll probably just revert this here.

@QuLogic
Copy link
Member Author

QuLogic commented Jun 30, 2021

It looks like the examples/shapes_and_collections/fancybox_demo.py change will conflict with #20538, but that's shaping it into a much better example, so I'll probably just revert this here.

I've reverted this part of the change, as that PR is much better.

fig = plt.figure(figsize=(12, 6))
sfs = fig.subfigures(1, 2)
fig = plt.figure(figsize=(6, 12))
sfs = fig.subfigures(2, 1)
Copy link
Member

Choose a reason for hiding this comment

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

The Size.Fixed is maybe too small now in this example?

Copy link
Member

Choose a reason for hiding this comment

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

... also perhaps useful to make the figure have a facecolor here to make it clear how large the figure is?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how instructive this example is at all. There's very little explanation.

We're moving things around here:

  • 3.4.2 had two separate examples.
  • devdocs has put this into two subfigures side-by-side as part of Support SubFigures in AxesDivider. #20073. I'm not convinced that was an improvement. It makes for one large code blob in which it is harder to see what is going where.
  • This now changes the subfigure layout to vertical. This is even worse, because you cannot get significant parts of the code and of the figure onto a decent screen simultaneously.

I propose to revert to the original separate figures (or any other way to make this more compact and understandable).

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

These all look better except perhaps for my comment...

fig = plt.figure(figsize=(12, 6))
sfs = fig.subfigures(1, 2)
fig = plt.figure(figsize=(6, 12))
sfs = fig.subfigures(2, 1)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how instructive this example is at all. There's very little explanation.

We're moving things around here:

  • 3.4.2 had two separate examples.
  • devdocs has put this into two subfigures side-by-side as part of Support SubFigures in AxesDivider. #20073. I'm not convinced that was an improvement. It makes for one large code blob in which it is harder to see what is going where.
  • This now changes the subfigure layout to vertical. This is even worse, because you cannot get significant parts of the code and of the figure onto a decent screen simultaneously.

I propose to revert to the original separate figures (or any other way to make this more compact and understandable).

@@ -30,7 +30,7 @@ def demo_con_style(ax, connectionstyle):
transform=ax.transAxes, ha="left", va="top")


fig, axs = plt.subplots(3, 5, figsize=(8, 4.8))
fig, axs = plt.subplots(3, 5, figsize=(7, 4), constrained_layout=True)
Copy link
Member

Choose a reason for hiding this comment

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

https://61514-1385122-gh.circle-artifacts.com/0/doc/build/html/gallery/userdemo/connectionstyle_demo.html#sphx-glr-gallery-userdemo-connectionstyle-demo-py

With the reduced size, the text becomes very dominating and visually too close to the arrows:
grafik
grafik

This needs more fine-tuning, i.e. moving points and/or changing font size and/or maxing out the available figure size.

I propose to hold off such difficult fine-tuning until we know what size will finally be available (xref pydata/pydata-sphinx-theme#427, pydata/pydata-sphinx-theme#428).

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @timhoffm on both accounts.

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 should be better now:
image

@jklymak
Copy link
Member

jklymak commented Jul 7, 2021

@QuLogic worst case scenario you could just revert that one example, and we could merge the rest of this?

@jklymak jklymak marked this pull request as draft July 7, 2021 20:36
QuLogic added 8 commits July 9, 2021 04:00
It is strange to have them all at the end "in" the Miscellaneous
section. And when sphinx-gallery sees multiple figures together, it
makes those into a "list" at half size, which makes them much too small
in the new theme.
Center the EEG image in sample plots tutorial, which matches the rest of
the images in the tutorial. Drop captions that don't say much, and make
images full size.
Place example images before API description, and don't scale down to
50%.
Don't scale to 50%, plus some small tweaks to demos so the images fit
better.
Select text colour based on luminance of the background, rather than
straight 50% black, which is invisible on some lines. Then tweak the
alignments settings to look nicer.
@QuLogic QuLogic force-pushed the fixup-tutorials branch from eaf6a6e to 0e5f9eb Compare July 9, 2021 08:15
@QuLogic QuLogic marked this pull request as ready for review July 9, 2021 08:15
@jklymak
Copy link
Member

jklymak commented Jul 19, 2021

I'll merge as an improvement. @timhoffm not 100% sure all your comments were addressed, but hopefully they can just be follow up.

@jklymak jklymak merged commit d686d73 into matplotlib:master Jul 19, 2021
@QuLogic QuLogic deleted the fixup-tutorials branch July 19, 2021 19:59
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