Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
TheSize.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 ofSupport 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 ofSupport 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 (xrefpydata/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.
This should be better now:
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@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 Miscellaneoussection. And when sphinx-gallery sees multiple figures together, itmakes those into a "list" at half size, which makes them much too smallin the new theme.
Center the EEG image in sample plots tutorial, which matches the rest ofthe images in the tutorial. Drop captions that don't say much, and makeimages full size.
Place example images before API description, and don't scale down to50%.
Don't scale to 50%, plus some small tweaks to demos so the images fitbetter.
Select text colour based on luminance of the background, rather thanstraight 50% black, which is invisible on some lines. Then tweak thealignments 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
pytestpasses).flake8on changed files to check).flake8-docstringsand 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).