revamp using style sheets #88

jimustafa · 2021-11-14T19:35:28Z

This is the beginning of a bigger effort to refactor the figure generation scripts to use style sheets. The goal is make the scripts a bit more DRY and to be able to enforce consistent styling of the figures.

There are a few more commits that I have ready to go, but wanted to get a discussion going here about whether this is a good path forward.

Part of the refactoring effort will be generating figures that have their final size in the compiled PDF. That way, no scaling is needed with \includegraphics and two different figures with the same font face and size will not have final size differences. Also, with style sheets, we can enforce constrained_layout=True and obviate the need for running pdfcrop which adds time to the build process.

jimustafa · 2021-11-15T08:40:58Z

Oops! on my part with the marking as draft/ready noise. @story645, did you think the approach proposed above is worth continuing? If so, I have more changes that can be added to this PR; it would make for a large PR...

jklymak · 2021-11-15T09:57:52Z

Personally I think this is a great way to go. I almost wonder about removing tex, but maybe that is a bridge too far...

story645 · 2021-11-15T14:03:55Z

Marked as ready was my mistake while looking for a lable. I think this could be a worthwhile approach, but if it's major should probably get sign off from @rougier

jimustafa · 2021-11-15T16:58:12Z

I will re-mark as draft before adding more commits, so that the changes could be discussed along the way.

Agreed @story645, will wait for feedback from @rougier. Ideally, this is a straight refactoring of the scripts for generating the figures, and will cause only minor visual changes to the cheatsheets and handouts.

@jklymak, it would be nice to do it all in python... Would it be straightforward to achieve the fancy layout of the cheatsheets?

jklymak · 2021-11-16T19:28:48Z

Would it be straightforward to achieve the fancy layout of the cheatsheets?

I doubt it is straightforward - it looks like the TeX has a lot of customization in it. OTOH, we now have subfigures in Matplotlib, so its not impossible.

rougier · 2021-11-22T14:10:50Z

Thanks for starting these changes. What you propose sounds goods, even though I'm not sure we can completely get rid of Larex it if we want nice text display in the handouts.

scripts/advanced-plots.py

lgtm-com · 2021-11-30T05:12:43Z

This pull request fixes 6 alerts when merging 1268667 into b1825a8 - view on LGTM.com

fixed alerts:

4 for Variable defined multiple times
2 for Unused import

jimustafa · 2021-11-30T05:13:47Z

This PR is at a good point for review. It may have went a bit overboard on the refactoring, with a lot of changes related to migrating to plt.subplots and making use of mpl.rc and subplot_kw to do some styling and axes configuration, respectively. All of the files that were touched should use the base style sheet, which enforces constrained_layout=True; other scripts may have their own custom style sheet, such as for the tick-*.py examples. A few scripts had considerably less refactoring, where it did not seem to be as straightforward.

Hopefully the size of the PR is not too off-putting... It could be broken into more manageable chunks, if necessary.

jklymak · 2021-11-30T08:36:15Z

scripts/extents.py

 Z = np.arange(5*5).reshape(5, 5)

 fig = plt.figure(figsize=(8, 5))
+gs = fig.add_gridspec(2, 2)


any reason to not just add these all at once: fig, axs = plt.subplots(2, 2, figsize=(8, 5))

This comment still stands - there is really very little reason for users to drop down into add_gridspec anymore.

I must have missed this comment during the previous review. Fixed.

jklymak

I skimmed this pretty quickly. I think this is in general a good way to go to minimize the pdfcrop step and encapsulate things a bit better.

I think what is needed is for @rougier to be OK with it - we dont' want him coming back to this and not being able to find anything!

rougier

Ok, the PR is a bit too big for me to review everything and the use of the stylesheet makes it a bit harder. If the output is approximately the same, I think we can merge it. I've only a few questions on some specific parameters that have been changed with no obvious reason (for me I mean).

rougier · 2021-12-15T10:56:37Z

scripts/adjustements.py



 box = mpatches.FancyBboxPatch(
    (0, 0), 100, 83, mpatches.BoxStyle("Round", pad=0, rounding_size=2),
-    linewidth=1., facecolor="0.9", edgecolor="black")
+    facecolor="0.9", edgecolor="black")


Why did you remove linewidth=1?

The linewidth is configured with mpl.rc.

rougier · 2021-12-15T10:57:28Z

scripts/adjustements.py

@@ -85,45 +105,44 @@ def int_arrow(p0, p1):
 x = 0
 y = 10
 ext_arrow( (x-4, y), (x, y), (x+5, y), (x+9, y) )
-ax.text(x+9.5, y, "left", ha="left", va="center", size="x-small", zorder=20)
+ax.text(x+9.5, y, "left", ha="left", va="center", zorder=20)


Why did you remove the text size?

The font size is configured with mpl.rc.

rougier · 2021-12-15T10:58:53Z

scripts/annotation-connection-styles.py

 demo_con_style(axs[0, 0], "arc3,rad=0")
 demo_con_style(axs[0, 1], "arc3,rad=0.3")
 demo_con_style(axs[0, 2], "angle3,angleA=0,angleB=90")
 demo_con_style(axs[1, 0], "angle,angleA=-90,angleB=180,rad=0")
-demo_con_style(axs[1, 1], "angle,angleA=-90,angleB=180,rad=25")


Any reason to change these values?

With the change in the figure size to prevent resizing in LaTeX, a couple of tweaks were needed to maintain the look of the annotations.

jimustafa · 2021-12-16T04:59:21Z

Thanks @rougier for the comments. Many of the kwargs style config have been moved to mpl.rc near the top of the file, right after where the style sheets are specified. In the case of adjustements.py, a file to which you refer, we have:

mpl.style.use([
    pathlib.Path(__file__).parent/'../styles/base.mplstyle',
])
mpl.rc('font', size=4)
mpl.rc('lines', linewidth=0.5)
mpl.rc('patch', linewidth=0.5)

This is meant to replace a lot of repetitive kwargs in the calls to ax.text.

jimustafa · 2021-12-16T06:21:31Z

Ok, the PR is a bit too big for me to review everything and the use of the stylesheet makes it a bit harder. If the output is approximately the same, I think we can merge it. I've only a few questions on some specific parameters that have been changed with no obvious reason (for me I mean).

Yes, admittedly, the PR did get a bit bigger than it probably should have. There are some slight visual differences in the cheatsheets and handouts, and I would appreciate any double-checking for gross differences that I might have missed.

@rougier @jklymak @timhoffm: could you help me with a side-by-side comparison of the cheatsheets and handouts built for this PR with those published at https://matplotlib.org/cheatsheets.

lgtm-com · 2021-12-16T06:27:28Z

This pull request introduces 1 alert and fixes 6 when merging 73ced9f into 6e1d7a9 - view on LGTM.com

new alerts:

1 for Unused import

fixed alerts:

4 for Variable defined multiple times
2 for Unused import

rougier · 2022-01-03T09:10:18Z

@jimustafa Would you have a link to the produced PDF by any chance?

jimustafa · 2022-01-03T16:26:49Z

@jimustafa Would you have a link to the produced PDF by any chance?

https://github.com/matplotlib/cheatsheets/suites/4674996554/artifacts/127245574

I also uploaded a small guide on finding the build artifacts for a PR (please see below).
finding-build-artifacts.pdf

rougier · 2022-01-03T19:53:55Z

Thanks. Some comments:

For tick locators /formatters, the labels are slightly too close to the axis, they are touching it while it would be better to keep a thin space.
The annotation are a bit "heavy" and arrows are not supposed to touch discs. I inserted some margins to ease reading but they dissapeared.

lgtm-com · 2022-01-03T21:28:34Z

This pull request introduces 1 alert and fixes 6 when merging 1132fc7 into 6e1d7a9 - view on LGTM.com

new alerts:

1 for Unused import

fixed alerts:

4 for Variable defined multiple times
2 for Unused import

Notes: The A4 long dimension is 297 mm. With 2.5 mm left and right margins and 7/4 mm spacing between the 5 columns (4 gaps), this gives (297 - 2.5*2 - 4*7/4) / 5 = 57 mm wide columns.

- add base style with constrained_layout enabled - add style for the ticks examples - add style for a grid of figures - resize figures to a maximum width of 57 mm - small tweaks to arrow connections to accommodate change in figure size

- diminish scope of pdfcrop use - migrate more scripts to use the base mplstyle - continue migration to subplots usage - refactor to use common subplot_kw where possible - add style sheet for the sine plots - make more DRY

lgtm-com · 2022-01-03T22:40:14Z

This pull request introduces 1 alert and fixes 6 when merging e4c6b67 into 653fc62 - view on LGTM.com

new alerts:

1 for Unused import

fixed alerts:

4 for Variable defined multiple times
2 for Unused import

rougier · 2022-01-04T14:50:47Z

I think we can merge now. Many thanks for the impressive work. This will make our life easier when updating.

jimustafa · 2023-03-30T00:25:33Z

It has been some time since the last discussion on this PR. With the recent activity on this repo, maybe it is a good time to revisit.

@rougier, @jklymak, do you still think this PR is suitable to be merged?

rougier · 2023-03-30T06:34:42Z

scripts/advanced-plots.py

-mpl.rcParams['xtick.major.size'] = 0.0
-mpl.rcParams['ytick.major.size'] = 0.0
-d = 0.01
-ax = fig.add_axes([d, d, 1-2*d, 1-2*d])


This ax is to make sure we have enough space for the frame lines. If set exactly to 0,0,1,1, then the outer lines are clipped and are not anti-aliased. Not sure the replacement code take care of that

Yes, the constrained_layout parameters in base.mplstyle include padding around the frame so that it is not clipped.

rougier · 2023-03-30T06:35:03Z

scripts/adjustements.py

 ax.add_artist(box)

 # Window button
 X, Y = [5, 10, 15], [79, 79, 79]
-plt.scatter(X, Y, s=75, zorder=10,
-            edgecolor="black", facecolor="white", linewidth=1)
+plt.scatter(X, Y, s=20, zorder=10,


Why change the size?

The size of the figures were reduced to match \columnwidth in cheatsheets.tex; so, the marker sizes were reduced accordingly.

rougier · 2023-03-30T06:35:58Z

scripts/advanced-plots.py

 X = np.linspace(0, 10, 16)
 Y = 4 + 2*np.sin(2*X)
-ax.step(X, Y, color="C1", linewidth=0.75)
-ax.set_xlim(0, 8), ax.set_xticks(np.arange(1, 8))


Is this set somewhere else ?

Yes, that is set in the plotlet.mplstyle style sheet.

rougier · 2023-03-30T06:37:16Z

scripts/annotation-arrow-styles.py



-(fig, axes) = plt.subplots(4, 4, figsize=(4, 2.5), frameon=False)
+(fig, axes) = plt.subplots(4, 4, figsize=(5.7/2.54, 1.5), frameon=True)


Why frameon=True?

rougier · 2023-03-30T06:37:48Z

scripts/annotation-arrow-styles.py

                                color="black",
                                shrinkA=5, shrinkB=5,
                                patchA=None, patchB=None,
                                connectionstyle="arc3,rad=0"))
    ax.text( (x1+x0)/2, y0-0.2, style,
             transform=ax.transAxes,
-             family="Source Code Pro", ha="center", va="top")


Why removed?

rougier · 2023-03-30T06:41:26Z

scripts/tick-formatters.py

-def major_formatter(x, pos):
-    """Return formatted value with 2 decimal places."""
-    return "[%.2f]" % x
+ax.text(0.0, 0.2, "ticker.FixedFormatter(['zero', 'one', 'two', …])",


Less readable I think (even if more compact)

rougier · 2023-03-30T06:53:59Z

I've added a few comments but overall if looks good. However, I've two major comments:

The code is now more compact but if you consider the "educative" nature to the cheat sheets, we need to find the right balance between compactness of the code and readability for a new user. If you want to re-use a script, you now have to find the relevant files that are loaded to get the same output. I agree it make our life easier to maintenance, but it hinders usability a bit.
I use this line quit extensively ax = fig.add_axes([d, d, 1-2*d, 1-2*d]) (with d =0.01) in order to have full ax while taking car of having spine lines not clipped (as it would be the case with ax=[0,0,1,1]. I was you replaced the code but I'm not sure if you took care of the clipping problem.

jimustafa · 2023-03-30T08:18:39Z

The code is now more compact but if you consider the "educative" nature to the cheat sheets, we need to find the right balance between compactness of the code and readability for a new user. If you want to re-use a script, you now have to find the relevant files that are loaded to get the same output. I agree it make our life easier to maintenance, but it hinders usability a bit.

I understand this concern. Yes, it does make the scripts a bit more abstract and may hinder direct reuse. In some sense, this new approach may also add some "educative" value as it provides an example of using style sheets to enforce consistent styling across multiple figure generation scripts.

2. I use this line quit extensively ax = fig.add_axes([d, d, 1-2*d, 1-2*d]) (with d =0.01) in order to have full ax while taking car of having spine lines not clipped (as it would be the case with ax=[0,0,1,1]. I was you replaced the code but I'm not sure if you took care of the clipping problem.

This type of padding has been handled with the constrained_layout parameters in the base.mplstyle and plotlet.mplstyle style sheets. Inspecting the built figures does show sufficient padding around the frame.

Also, I reverted some changes for a few scripts where you commented on changes for which I do not remember the rationale.

rougier · 2023-03-30T12:35:09Z

Thanks for your anwer and your work. Good for me (again)!

rougier · 2023-03-30T12:35:42Z

If no further comment from @jklymak I guess we can merge.

jklymak · 2023-03-30T14:47:04Z

scripts/sine.py

-plt.tight_layout()
-plt.savefig("../figures/sine-legend.pdf", dpi=100)
-# plt.show()
+(fig, ax) = plt.subplots(figsize=(5.7/2.54, 1.0/2.54))


There are a lot of repeated figure sizes with the same width (5.7/2.54 = 2.444). Does it make sense to define this once as a global? Maybe height as a global as well, and then work on multiples of the height? Otherwise this seems kind of messy to have this odd number everywhere.

Yeah, I do not like it much either. These expression came out of trying to be explicit and precise about the size as it is derived from the \columnwidth in the TeX. Now that #129 introduced a module import to each script for customizing the fonts, maybe we piggyback on that idea and develop a common configuration module. Or, figure sizes can be specified in a corresponding style sheet.

Do we merge this PR and you make another one for that?

Yes thats fine w/ me.

story645 marked this pull request as ready for review November 14, 2021 19:53

jimustafa marked this pull request as draft November 15, 2021 08:18

jimustafa marked this pull request as ready for review November 15, 2021 08:19

jimustafa marked this pull request as draft November 15, 2021 16:58

jimustafa force-pushed the revamp-with-style-sheets branch from dbf62a3 to 9a86821 Compare November 16, 2021 07:44

jimustafa mentioned this pull request Nov 18, 2021

refactor handout-tips scripts #94

Closed

rougier reviewed Nov 22, 2021

View reviewed changes

scripts/advanced-plots.py Show resolved Hide resolved

jimustafa force-pushed the revamp-with-style-sheets branch 2 times, most recently from 3a02535 to 32011c6 Compare November 24, 2021 04:42

jimustafa marked this pull request as ready for review November 30, 2021 05:13

jklymak reviewed Nov 30, 2021

View reviewed changes

jklymak approved these changes Nov 30, 2021

View reviewed changes

rougier reviewed Dec 15, 2021

View reviewed changes

jimustafa force-pushed the revamp-with-style-sheets branch from 1268667 to 73ced9f Compare December 16, 2021 06:09

jimustafa mentioned this pull request Jan 3, 2022

Update for version 3.5.0 #102

Merged

jimustafa added 5 commits January 3, 2022 14:14

make cheatsheets column width exactly 57 mm

aec0df4

Notes: The A4 long dimension is 297 mm. With 2.5 mm left and right margins and 7/4 mm spacing between the 5 columns (4 gaps), this gives (297 - 2.5*2 - 4*7/4) / 5 = 57 mm wide columns.

start refactoring using style sheets

b20eb40

continue refactoring using style sheets

5ea8b48

- add base style with constrained_layout enabled - add style for the ticks examples - add style for a grid of figures - resize figures to a maximum width of 57 mm - small tweaks to arrow connections to accommodate change in figure size

finish (for now) refactoring using style sheets

aeeeb3b

- diminish scope of pdfcrop use - migrate more scripts to use the base mplstyle - continue migration to subplots usage - refactor to use common subplot_kw where possible - add style sheet for the sine plots - make more DRY

small tweaks to size and spacing identified during review

e4c6b67

jimustafa force-pushed the revamp-with-style-sheets branch from 1132fc7 to e4c6b67 Compare January 3, 2022 22:18

rougier approved these changes Jan 4, 2022

View reviewed changes

jimustafa mentioned this pull request Mar 29, 2023

Save figures in a path relative to the script #127

Merged

rougier reviewed Mar 30, 2023

View reviewed changes

revert some changes

c44d32b

jklymak reviewed Mar 30, 2023

View reviewed changes

replace add_gridspec with plt.subplots

cfa709d

jklymak merged commit 36607cd into matplotlib:master Mar 31, 2023



		(fig, axes) = plt.subplots(4, 4, figsize=(4, 2.5), frameon=False)
		(fig, axes) = plt.subplots(4, 4, figsize=(5.7/2.54, 1.5), frameon=True)

revamp using style sheets #88

revamp using style sheets #88

Conversation

jimustafa commented Nov 14, 2021

jimustafa commented Nov 15, 2021

jklymak commented Nov 15, 2021

story645 commented Nov 15, 2021

jimustafa commented Nov 15, 2021

jklymak commented Nov 16, 2021

rougier commented Nov 22, 2021

lgtm-com bot commented Nov 30, 2021

jimustafa commented Nov 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

rougier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimustafa commented Dec 16, 2021

jimustafa commented Dec 16, 2021

lgtm-com bot commented Dec 16, 2021

rougier commented Jan 3, 2022

jimustafa commented Jan 3, 2022 • edited

rougier commented Jan 3, 2022

lgtm-com bot commented Jan 3, 2022

lgtm-com bot commented Jan 3, 2022

rougier commented Jan 4, 2022

jimustafa commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rougier commented Mar 30, 2023

jimustafa commented Mar 30, 2023

rougier commented Mar 30, 2023

rougier commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimustafa commented Jan 3, 2022 •

edited