-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
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. |
It looks like the |
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this 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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the reduced size, the text becomes very dominating and visually too close to the arrows:
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuLogic worst case scenario you could just revert that one example, and we could merge the rest of this? |
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.
I'll merge as an improvement. @timhoffm not 100% sure all your comments were addressed, but hopefully they can just be follow up. |
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:
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)
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).