cheatsheets icon indicating copy to clipboard operation
cheatsheets copied to clipboard

revamp using style sheets

Open jimustafa opened this issue 3 years ago • 17 comments

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 avatar Nov 14 '21 19:11 jimustafa

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...

jimustafa avatar Nov 15 '21 08:11 jimustafa

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

jklymak avatar Nov 15 '21 09:11 jklymak

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

story645 avatar Nov 15 '21 14:11 story645

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?

jimustafa avatar Nov 15 '21 16:11 jimustafa

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.

jklymak avatar Nov 16 '21 19:11 jklymak

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.

rougier avatar Nov 22 '21 14:11 rougier

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

lgtm-com[bot] avatar Nov 30 '21 05:11 lgtm-com[bot]

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.

jimustafa avatar Nov 30 '21 05:11 jimustafa

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 avatar Dec 16 '21 04:12 jimustafa

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.

jimustafa avatar Dec 16 '21 06:12 jimustafa

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

lgtm-com[bot] avatar Dec 16 '21 06:12 lgtm-com[bot]

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

rougier avatar Jan 03 '22 09:01 rougier

@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

jimustafa avatar Jan 03 '22 16:01 jimustafa

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.

rougier avatar Jan 03 '22 19:01 rougier

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

lgtm-com[bot] avatar Jan 03 '22 21:01 lgtm-com[bot]

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

lgtm-com[bot] avatar Jan 03 '22 22:01 lgtm-com[bot]

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

rougier avatar Jan 04 '22 14:01 rougier

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?

jimustafa avatar Mar 30 '23 00:03 jimustafa

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

  1. 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.

  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.

rougier avatar Mar 30 '23 06:03 rougier

  1. 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.

jimustafa avatar Mar 30 '23 08:03 jimustafa

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

rougier avatar Mar 30 '23 12:03 rougier

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

rougier avatar Mar 30 '23 12:03 rougier