Skip to content

Chapter 10: Adding a Section on Matplotlib Style Sheets#209

Merged
jstac merged 21 commits intomainfrom
style-sheet
Aug 30, 2022
Merged

Chapter 10: Adding a Section on Matplotlib Style Sheets#209
jstac merged 21 commits intomainfrom
style-sheet

Conversation

@HumphreyYang
Copy link
Copy Markdown
Member

Hi @jstac,

This PR resolves #203 . The content for this lecture looks good to me so I did not make other changes to this lecture.

Could you please kindly review these changes and let me know if there is any change/improvement I can make on this PR?

Many thanks.

@jstac
Copy link
Copy Markdown
Contributor

jstac commented Aug 28, 2022

Thanks @HumphreyYang .

@mmcky , I can't see a link to the deployment for this PR. Am I missing something obvious? Thanks.

@mmcky
Copy link
Copy Markdown
Contributor

mmcky commented Aug 28, 2022

@jstac there is a new category of contributor I haven't seen before Collaborator (External) -- maybe @HumphreyYang needs to be part of the QuantEcon organisation for those permissions now?

Although the build is running

https://6308b4bdea2ab00b79b91fd8--epic-agnesi-957267.netlify.app/intro.html

@mmcky
Copy link
Copy Markdown
Contributor

mmcky commented Aug 28, 2022

@HumphreyYang I have sent you an invite for the QuantEcon organisation (GitHub) as a member. Will see if that works?

@HumphreyYang
Copy link
Copy Markdown
Member Author

@HumphreyYang I have sent you an invite for the QuantEcon organisation (GitHub) as a member. Will see if that works?

Thanks @mmcky, I have accepted the invitation.

@jstac
Copy link
Copy Markdown
Contributor

jstac commented Aug 28, 2022

Thanks @mmcky and @HumphreyYang

@HumphreyYang , this looks good. Many thanks.

Is it possible to get the title centered and in space above the plots, so it's easier to see?

Please reduce the figsize to (10, 3).

@HumphreyYang
Copy link
Copy Markdown
Member Author

Thanks @mmcky and @HumphreyYang

@HumphreyYang , this looks good. Many thanks.

Is it possible to get the title centered and in space above the plots, so it's easier to see?

Please reduce the figsize to (10, 3).

Thank you so much for your feedback @jstac. I have centered the title and resized the figure.

Please kindly check the latest deployment and merge if it looks good to you.

Many thanks.

@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 02:16 Inactive
@jstac
Copy link
Copy Markdown
Contributor

jstac commented Aug 29, 2022

Thanks @HumphreyYang . Can you also please set the seed in these simulations (or change the logic) so that each plot acts on exactly the same data.

I'm also wondering if it wouldn't be easier for the reader to follow if we broke this up into 4 separate cells.

For example, you could have a function that takes a style as an argument and generates the plot.

Then you could call that function 4 times, with a little bit more explanation regarding what you are doing (holding the reader's hand).

What do you think?

@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 05:54 Inactive
@jstac
Copy link
Copy Markdown
Contributor

jstac commented Aug 29, 2022

Nice work @HumphreyYang .

Can you please separate these calls into separate cells:

draw_graphs(style = 'seaborn')
draw_graphs(style = 'grayscale')
draw_graphs(style = 'ggplot')
draw_graphs(style = 'dark_background')

I don't think you need both lines in

    random.seed(1)
    np.random.seed(1)

Finally, "let's" should be capitalized.

@HumphreyYang
Copy link
Copy Markdown
Member Author

HumphreyYang commented Aug 29, 2022

Thank you so much for the review @jstac.

I was catching an appointment so I haven't had everything finalised

I think

 random.seed(1)
 np.random.seed(1)

are needed to produce identical plots, but with random.seed(1) alone, the sampling variation of normal distribution does not have a significant impact on the plot.

I have made another push based on your review. Please kindly check the commit and merge if it looks good to you.

Many thanks.

@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 09:02 Inactive
@jstac
Copy link
Copy Markdown
Contributor

jstac commented Aug 29, 2022

@HumphreyYang , I see what you mean about the seed. I think it would be neatest if all random variables were drawn from np.random and we set the seed with np.random.seed(1).

Also, could you please put these function calls in separate cells. (At the moment they are all in the same cell.)

draw_graphs(style = 'seaborn')
draw_graphs(style = 'grayscale')
draw_graphs(style = 'ggplot')
draw_graphs(style = 'dark_background')

Please also remove the space around the = sign (which is the convention when = is used inside a function call or signature.

@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 09:15 Inactive
@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 09:18 Inactive
@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 09:22 Inactive
@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 09:47 Inactive
@HumphreyYang
Copy link
Copy Markdown
Member Author

HumphreyYang commented Aug 29, 2022

Thanks for the timely review @jstac

@HumphreyYang , I see what you mean about the seed. I think it would be neatest if all random variables were drawn from np.random and we set the seed with np.random.seed(1).

Thank you so much for pointing this out. I was too focused on using what they have learnt in this chapter to draw the graph

Also, could you please put these function calls in separate cells. (At the moment they are all in the same cell.)

draw_graphs(style = 'seaborn') draw_graphs(style = 'grayscale') draw_graphs(style = 'ggplot') draw_graphs(style = 'dark_background')
Please also remove the space around the = sign (which is the convention when = is used inside a function call or signature.

I have updated them to reflect these changes. Please kindly see the latest deployment here

@jstac
Copy link
Copy Markdown
Contributor

jstac commented Aug 29, 2022

Thanks @HumphreyYang , nice PR, but the calls to draw_graphs are all still in the same cell.

Please put them in separate cells, perhaps with some text above each one. (e.g., "First we call draw_graphs with style xyz...")

@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 23:12 Inactive
@HumphreyYang
Copy link
Copy Markdown
Member Author

Hi @jstac,

Sorry for misunderstanding your request. I thought you were asking me to add space between statements.

I have put these statements into separate cells. Please kindly check the latest commit.

@jstac
Copy link
Copy Markdown
Contributor

jstac commented Aug 29, 2022

Thanks @HumphreyYang .

Please change " to set style sheet." -> " to set the style sheet."

@github-actions github-actions Bot temporarily deployed to commit August 29, 2022 23:59 Inactive
@github-actions github-actions Bot temporarily deployed to commit August 30, 2022 07:11 Inactive
@HumphreyYang
Copy link
Copy Markdown
Member Author

HumphreyYang commented Aug 30, 2022

Hi @jstac,

Please kindly check the latest commits for the revised version of this PR, and merge if it looks good to you now.

Many thanks

@github-actions github-actions Bot temporarily deployed to commit August 30, 2022 07:46 Inactive
@github-actions github-actions Bot temporarily deployed to commit August 30, 2022 07:47 Inactive
@jstac
Copy link
Copy Markdown
Contributor

jstac commented Aug 30, 2022

Nice work @HumphreyYang , merging.

@jstac jstac merged commit de23d8e into main Aug 30, 2022
@jstac jstac deleted the style-sheet branch August 30, 2022 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding Style Sheets to Chapter 10 (Matplotlib)

3 participants