Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[prob_dist] Update suggestions #519

Merged
merged 5 commits into from
Jul 23, 2024
Merged

[prob_dist] Update suggestions #519

merged 5 commits into from
Jul 23, 2024

Conversation

longye-tian
Copy link
Collaborator

Dear John @jstac ,

This pull request update prob_dist.md according to #402 .

In particular, this pull request

  • update typo "The the " to 'The'
  • Make the code in Poisson distribution more concise by deleting some repeated code
  • Add x,y labels to all the plots

Best ❤️
Longye

Copy link

netlify bot commented Jul 22, 2024

Deploy Preview for taupe-gaufre-c4e660 ready!

Name Link
🔨 Latest commit 6574942
🔍 Latest deploy log https://app.netlify.com/sites/taupe-gaufre-c4e660/deploys/669f0664a6e0c900080f0a90
😎 Deploy Preview https://deploy-preview-519--taupe-gaufre-c4e660.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Jul 22, 2024

@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 00:32 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 00:34 Inactive
@longye-tian longye-tian requested review from jstac and mmcky July 22, 2024 00:39
lectures/prob_dist.md Outdated Show resolved Hide resolved
lectures/prob_dist.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mmcky mmcky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @longye-tian.

I think we should use lower case for the text titles on the x and y axis. CDF and PDF are acronyms so I think they should remain capitalised but I will confirm with @jstac

https://manual.quantecon.org/styleguide/figures.html

Co-authored-by: Matt McKay <mmcky@users.noreply.github.com>
@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 01:13 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 01:16 Inactive
@longye-tian
Copy link
Collaborator Author

thanks @longye-tian.

I think we should use lower case for the text titles on the x and y axis. CDF and PDF are acronyms so I think they should remain capitalised but I will confirm with @jstac

https://manual.quantecon.org/styleguide/figures.html

Hi Matt,

Thanks! Just updated the labels to lowercase letters except the PMF, CDF, PDF and KDE.

Best,
Longye

@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 01:34 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 01:35 Inactive
@mmcky
Copy link
Contributor

mmcky commented Jul 22, 2024

thanks @longye-tian.

@jstac this is ready for your final review. It is my understanding that acronyms like PDF should be capitalized. Is that correct?

lectures/prob_dist.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mmcky mmcky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @longye-tian

@github-actions github-actions bot temporarily deployed to pull request July 23, 2024 01:31 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 23, 2024 01:32 Inactive
@mmcky
Copy link
Contributor

mmcky commented Jul 23, 2024

thanks @longye-tian for these updates. I will merge this now.

@mmcky mmcky merged commit e809a43 into main Jul 23, 2024
7 checks passed
@mmcky mmcky deleted the prob_dist_update branch July 23, 2024 02:46
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.

2 participants