refactor handout-tips scripts by jimustafa · Pull Request #94 · matplotlib/cheatsheets

jimustafa · 2021-11-18T08:28:53Z

combine all the handout-tips scripts into one
expected to help with revamp using style sheets #88

lgtm-com · 2021-11-18T08:48:28Z

This pull request introduces 3 alerts and fixes 5 when merging fa241e2 into 5f45dcb - view on LGTM.com

new alerts:

3 for Variable defined multiple times

fixed alerts:

4 for Unused import
1 for Variable defined multiple times

scripts/tips.py

lgtm-com · 2021-11-19T02:56:19Z

This pull request fixes 5 alerts when merging 7224cc5 into 5f45dcb - view on LGTM.com

fixed alerts:

4 for Unused import
1 for Variable defined multiple times

rougier · 2021-11-22T17:32:00Z

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?

timhoffm · 2021-11-22T18:15:42Z

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.

rougier · 2021-11-22T18:22:13Z

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.

jimustafa · 2021-11-23T01:33:14Z

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.

lgtm-com · 2021-11-23T07:50:31Z

This pull request fixes 5 alerts when merging e204e24 into c26b5c4 - view on LGTM.com

fixed alerts:

4 for Unused import
1 for Variable defined multiple times

- can help make things a bit more DRY - remove unused tip-post-processing.py script

- replace calls to `plt` with `fig` or `ax` equivalents

QuLogic

Mostly minor things.

But also, I see tip-post-processing.py was deleted, and I don't see it in the merged script?

QuLogic · 2023-03-29T07:47:09Z

scripts/tips.py

+fig.patch.set_alpha(0.0)
+n = 1
+
+fontsize = 18


QuLogic · 2023-03-29T07:47:39Z

scripts/tips.py

+fig = plt.figure(figsize=(5, 0.5))
+fig.patch.set_alpha(0.0)
+n = 1
+
+fontsize = 18
+ax = plt.subplot(n, 1, 1)


Join fig, and ax, I think?

Suggested change

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

QuLogic · 2023-03-29T07:48:06Z

scripts/tips.py

+# Setup a plot such that only the bottom spine is shown
+def setup(ax):


Suggested change

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

QuLogic · 2023-03-29T07:48:45Z

scripts/tips.py

+    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)


Suggested change

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)

QuLogic · 2023-03-29T07:49:44Z

scripts/tips.py

+fig = plt.figure(figsize=(2, 2))
+ax = plt.subplot()


Suggested change

fig = plt.figure(figsize=(2, 2))

ax = plt.subplot()

fig, ax = plt.subplots(figsize=(2, 2))

QuLogic · 2023-03-29T07:56:08Z

scripts/tips.py

+fig = plt.figure(figsize=(2, 2), dpi=100)
+ax = fig.add_axes(rect)
+n = 500
+np.random.seed(5)


Suggested change

np.random.seed(5)

np.random.seed(123)

rougier · 2023-03-29T18:08:43Z

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.

jimustafa · 2023-03-30T00:17:20Z

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!

QuLogic reviewed Nov 18, 2021

View reviewed changes

scripts/tips.py Outdated Show resolved Hide resolved

scripts/tips.py Outdated Show resolved Hide resolved

scripts/tips.py Outdated Show resolved Hide resolved

scripts/tips.py Outdated Show resolved Hide resolved

jimustafa force-pushed the refactor-tips branch from 7224cc5 to 5c7deb3 Compare November 23, 2021 07:30

jimustafa added 5 commits March 29, 2023 00:11

combine all tips scripts into one

c7f61f5

- can help make things a bit more DRY - remove unused tip-post-processing.py script

refactor tips

03e015b

fix whitespace and other formatting

7d9ab77

refactor tips further

a5a1d75

- replace calls to `plt` with `fig` or `ax` equivalents

fixes for PEP-8

291f79e

jimustafa force-pushed the refactor-tips branch from e204e24 to 291f79e Compare March 29, 2023 07:11

QuLogic reviewed Mar 29, 2023

View reviewed changes

jimustafa closed this Mar 30, 2023

		# Setup a plot such that only the bottom spine is shown
		def setup(ax):

	fig = plt.figure(figsize=(2, 2))
	ax = plt.subplot()
	fig, ax = plt.subplots(figsize=(2, 2))

Conversation

jimustafa commented Nov 18, 2021

Uh oh!

lgtm-com bot commented Nov 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Nov 19, 2021

Uh oh!

rougier commented Nov 22, 2021

Uh oh!

timhoffm commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rougier commented Nov 22, 2021

Uh oh!

jimustafa commented Nov 23, 2021

Uh oh!

lgtm-com bot commented Nov 23, 2021

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

QuLogic Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

QuLogic Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

QuLogic Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

QuLogic Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

QuLogic Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

rougier commented Mar 29, 2023

Uh oh!

jimustafa commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timhoffm commented Nov 22, 2021 •

edited

Loading

QuLogic Mar 29, 2023 •

edited

Loading