-
Notifications
You must be signed in to change notification settings - Fork 898
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
revamp using style sheets #88
revamp using style sheets #88
Conversation
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... |
Personally I think this is a great way to go. I almost wonder about removing tex, but maybe that is a bridge too far... |
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 |
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? |
dbf62a3
to
9a86821
Compare
I doubt it is straightforward - it looks like the TeX has a lot of customization in it. OTOH, we now have |
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. |
3a02535
to
32011c6
Compare
This pull request fixes 6 alerts when merging 1268667 into b1825a8 - view on LGTM.com fixed alerts:
|
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 Hopefully the size of the PR is not too off-putting... It could be broken into more manageable chunks, if necessary. |
scripts/extents.py
Outdated
Z = np.arange(5*5).reshape(5, 5) | ||
|
||
fig = plt.figure(figsize=(8, 5)) | ||
gs = fig.add_gridspec(2, 2) |
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.
any reason to not just add these all at once: fig, axs = plt.subplots(2, 2, figsize=(8, 5))
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 comment still stands - there is really very little reason for users to drop down into add_gridspec
anymore.
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 must have missed this comment during the previous review. Fixed.
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 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!
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.
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).
|
||
|
||
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") |
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.
Why did you remove linewidth=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 linewidth is configured with mpl.rc.
@@ -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) |
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.
Why did you remove the text size?
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 font size is configured with mpl.rc.
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") |
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.
Any reason to change these values?
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 change in the figure size to prevent resizing in LaTeX, a couple of tweaks were needed to maintain the look of the annotations.
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 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 |
1268667
to
73ced9f
Compare
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. |
This pull request introduces 1 alert and fixes 6 when merging 73ced9f into 6e1d7a9 - view on LGTM.com new alerts:
fixed alerts:
|
@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). |
Thanks. Some comments:
|
This pull request introduces 1 alert and fixes 6 when merging 1132fc7 into 6e1d7a9 - view on LGTM.com new alerts:
fixed alerts:
|
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
1132fc7
to
e4c6b67
Compare
This pull request introduces 1 alert and fixes 6 when merging e4c6b67 into 653fc62 - view on LGTM.com new alerts:
fixed alerts:
|
I think we can merge now. Many thanks for the impressive work. This will make our life easier when updating. |
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]) |
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 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
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.
Yes, the constrained_layout
parameters in base.mplstyle
include padding around the frame so that it is not clipped.
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, |
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.
Why change the size?
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 of the figures were reduced to match \columnwidth
in cheatsheets.tex
; so, the marker sizes were reduced accordingly.
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)) |
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.
Is this set somewhere else ?
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.
Yes, that is set in the plotlet.mplstyle
style sheet.
scripts/annotation-arrow-styles.py
Outdated
|
||
|
||
(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) |
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.
Why frameon=True?
scripts/annotation-arrow-styles.py
Outdated
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") |
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.
Why removed?
scripts/tick-formatters.py
Outdated
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', …])", |
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.
Less readable I think (even if more compact)
I've added a few comments but overall if looks good. However, I've two major comments:
|
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.
This type of padding has been handled with the Also, I reverted some changes for a few scripts where you commented on changes for which I do not remember the rationale. |
Thanks for your anwer and your work. Good for me (again)! |
If no further comment from @jklymak I guess we can merge. |
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 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.
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.
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.
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.
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.
Do we merge this PR and you make another one for that?
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.
Yes thats fine w/ me.
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 enforceconstrained_layout=True
and obviate the need for runningpdfcrop
which adds time to the build process.