refactor handout-tips scripts #94
Conversation
jimustafa
commented
Nov 18, 2021
jimustafa
commented
- combine all the handout-tips scripts into one
- expected to help with revamp using style sheets #88
|
This pull request introduces 3 alerts and fixes 5 when merging fa241e2 into 5f45dcb - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request fixes 5 alerts when merging 7224cc5 into 5f45dcb - view on LGTM.com fixed alerts:
|
|
I prefer to have one file / one idea because I find difficult some time to get the code corresponding to an image when there are several examples in the same file (e.g. gallery). Furthermore, what do we gain by having a single file? |
|
I don't have a strong opinion either way. Out of curiosity: what makes multiple files more manageable? The main obstacle is finding the appropriate name for a figure. Whether I then open [name].py or search for "[name].pdf" (from the savefig command) in a single file is no big difference. |
|
Look for example https://matplotlib.org/2.0.2/examples/statistics/boxplot_demo.html. Source is compact but as a user, if you're interested in a specific subfigure, it takes your some time to identify the code. Having one script / figure make it easier to get the relevant code. |
|
Admittedly, consolidating the scripts is probably just a matter of taste. There may be some benefit in being able to more easily recognize code patterns and keeping things DRY. Could also be easier to keep a consistent style with a single file. Either way, not a big deal. |
jimustafa
force-pushed
the
refactor-tips
branch
from
7224cc5 to
5c7deb3
Compare
|
This pull request fixes 5 alerts when merging e204e24 into c26b5c4 - view on LGTM.com fixed alerts:
|
- can help make things a bit more DRY - remove unused tip-post-processing.py script
- replace calls to `plt` with `fig` or `ax` equivalents
jimustafa
force-pushed
the
refactor-tips
branch
from
e204e24 to
291f79e
Compare
| fig.patch.set_alpha(0.0) | ||
| n = 1 | ||
|
|
||
| fontsize = 18 |
| fig = plt.figure(figsize=(5, 0.5)) | ||
| fig.patch.set_alpha(0.0) | ||
| n = 1 | ||
|
|
||
| fontsize = 18 | ||
| ax = plt.subplot(n, 1, 1) |
There was a problem hiding this comment.
Join fig, and ax, I think?
| fig = plt.figure(figsize=(5, 0.5)) | |
| fig.patch.set_alpha(0.0) | |
| n = 1 | |
| fontsize = 18 | |
| ax = plt.subplot(n, 1, 1) | |
| fig, ax = plt.subplots(figsize=(5, 0.5)) | |
| fig.patch.set_alpha(0.0) | |
| fontsize = 18 |
| # Setup a plot such that only the bottom spine is shown | ||
| def setup(ax): |
There was a problem hiding this comment.
| # Setup a plot such that only the bottom spine is shown | |
| def setup(ax): | |
| def setup(ax): | |
| """Setup a plot such that only the bottom spine is shown.""" |
| ax.tick_params(which='major', width=1.00) | ||
| ax.tick_params(which='major', length=5) | ||
| ax.tick_params(which='minor', width=0.75) | ||
| ax.tick_params(which='minor', length=2.5) |
There was a problem hiding this comment.
| ax.tick_params(which='major', width=1.00) | |
| ax.tick_params(which='major', length=5) | |
| ax.tick_params(which='minor', width=0.75) | |
| ax.tick_params(which='minor', length=2.5) | |
| ax.tick_params(which='major', width=1.0, length=5) | |
| ax.tick_params(which='minor', width=0.75, length=2.5) |
| fig = plt.figure(figsize=(2, 2)) | ||
| ax = plt.subplot() |
There was a problem hiding this comment.
| fig = plt.figure(figsize=(2, 2)) | |
| ax = plt.subplot() | |
| fig, ax = plt.subplots(figsize=(2, 2)) |
| fig = plt.figure(figsize=(2, 2), dpi=100) | ||
| ax = fig.add_axes(rect) | ||
| n = 500 | ||
| np.random.seed(5) |
There was a problem hiding this comment.
| np.random.seed(5) | |
| np.random.seed(123) |
|
Again, I don't see the advantage of having a single script. I think it is easier to have well named isolated scripts that make things easier to find for the user. |
I get it. Was not sure a decision was made on this PR, so I just rebased so that it could be merged. Combining the scripts could help reduce repetition, especially as we add more per-script configuration, as in #127 and #129. Anyways, I will close for now, and can reopen if opinion changes. Thanks! |
