cheatsheets
cheatsheets copied to clipboard
revamp using style sheets
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.
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?
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.
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.
This pull request fixes 6 alerts when merging 1268667968be4b827dae1f9f24b731d1955fa173 into b1825a80284ebf481e3c71a9352d3e13879648aa - view on LGTM.com
fixed alerts:
- 4 for Variable defined multiple times
- 2 for Unused import
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.
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
.
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.
This pull request introduces 1 alert and fixes 6 when merging 73ced9f351b2f2b55b1ed173e9a6462bf1b455b8 into 6e1d7a9a7458c995c1dbe29ae202c0ffff4bd1f0 - view on LGTM.com
new alerts:
- 1 for Unused import
fixed alerts:
- 4 for Variable defined multiple times
- 2 for Unused import
@jimustafa Would you have a link to the produced PDF by any chance?
@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
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.
This pull request introduces 1 alert and fixes 6 when merging 1132fc7dd498de1b712e538386cb2478513100f0 into 6e1d7a9a7458c995c1dbe29ae202c0ffff4bd1f0 - view on LGTM.com
new alerts:
- 1 for Unused import
fixed alerts:
- 4 for Variable defined multiple times
- 2 for Unused import
This pull request introduces 1 alert and fixes 6 when merging e4c6b67c9520d67442d616a220b5092741586e56 into 653fc62862905cfbe82b61555fd501fc42eccbf4 - view on LGTM.com
new alerts:
- 1 for Unused import
fixed alerts:
- 4 for Variable defined multiple times
- 2 for Unused import
I think we can merge now. Many thanks for the impressive work. This will make our life easier when updating.
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?
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 withax=[0,0,1,1]
. I was you replaced the code but I'm not sure if you took care of the clipping problem.
- 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 withax=[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.
Thanks for your anwer and your work. Good for me (again)!
If no further comment from @jklymak I guess we can merge.