Skip to content

Drop in Unidata matplotlib workshop notebook #37

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

Merged
merged 13 commits into from
Jun 15, 2021

Conversation

dcamron
Copy link
Contributor

@dcamron dcamron commented Apr 20, 2021

This is a drop-in of this notebook from Unidata's workshop materials with exercises modified and branding removed and some minor image updates. Otherwise mostly content-unmodified.

Notes:

  • Looks pretty good, organized similarly to others
  • Relatively small/simple scope, little crossover to other applications. Might fit in to a larger unit or serves as decent intro
  • As with some of the others I've dropped in, ties in pretty heavily to Meteorology concepts/jargon

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 54870a5 at: https://607f4d63fcedb613bdac11da--pythia-foundations.netlify.app

@dcamron dcamron added the content Content related issue label Apr 20, 2021
@clyne clyne added this to the InitialPortalRelease milestone Jun 4, 2021
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: f8acb0f at: https://60c246c43f93080d185cdd1b--pythia-foundations.netlify.app

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 28c27f0 at: https://60c272dcc3d3ef3875570fd6--pythia-foundations.netlify.app

@mgrover1 mgrover1 requested review from a team, ktyle and clyne and removed request for a team June 10, 2021 20:26
@brian-rose
Copy link
Member

I'm not on the reviewers list, but I took a quick look and there's great content here!

Two quick issues:

  • The "Notes" and other admonitions are rendering weirdly in the book
  • Would it be possible to break up the content into a few more major section headings ##? Currently essentially the whole content falls under one heading "Plotting with Matplotlib"

@clyne
Copy link
Contributor

clyne commented Jun 11, 2021

Looks great. I just had a few nits.

@clyne
Copy link
Contributor

clyne commented Jun 14, 2021

@mgrover1 just checking in to see if you think this is still doable for wednesday. Thanks!

@mgrover1
Copy link
Contributor

@mgrover1 just checking in to see if you think this is still doable for wednesday. Thanks!

Yes - sorry I will try to push the new changes today... busy with the CESM workshop this week, but I will get those changes made so you all can take a look and we can get this merged

@clyne
Copy link
Contributor

clyne commented Jun 14, 2021

Coolio. Thanks!

@mgrover1
Copy link
Contributor

All suggestions have been addressed I believe - would appreciate another look-over from @ktyle and @clyne

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 2022cc0 at: https://60c7cbbbd73be21a19ea3998--pythia-foundations.netlify.app

@clyne
Copy link
Contributor

clyne commented Jun 14, 2021

On it! FYI you can trigger a re-review whenever you are ready by clicking the circular arrows next to the reviewers names.

@mgrover1 mgrover1 requested review from ktyle and clyne June 14, 2021 21:38
Copy link
Contributor

@clyne clyne left a comment

Choose a reason for hiding this comment

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

This is awesome, Max!

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 5c50860 at: https://60c811626f2d4765312ebdab--pythia-foundations.netlify.app

@mgrover1
Copy link
Contributor

@ktyle I went ahead and fixed the spelling - thanks for the comments!

@ktyle
Copy link
Contributor

ktyle commented Jun 15, 2021

@mgrover1 great! I think we're just about there! The only other thing I would recommend is that you de-emphasize the "exercise-like" instructions that proceed one of the last notebooks ... maybe just delete the "Try to" in the numbered item that specifies adding a colorbar.

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 72f3c67 at: https://60c8ab8280fb6cf783229aec--pythia-foundations.netlify.app

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: c7210af at: https://60c8adf42601a706497433e9--pythia-foundations.netlify.app

@mgrover1
Copy link
Contributor

@ktyle I made that change

@ktyle
Copy link
Contributor

ktyle commented Jun 15, 2021

LGTM and I think this is ready to merge ...

@mgrover1
Copy link
Contributor

@ktyle Awesome! Can you hit the approve button?

@ktyle
Copy link
Contributor

ktyle commented Jun 15, 2021

@mgrover1 by "Approve" do you mean "Squash and merge"?

@mgrover1
Copy link
Contributor

@ktyle Sure - that or the "formal review" at the top of the page? Wasn't sure if we technically needed two approvals to merge

@brian-rose brian-rose merged commit 6023d18 into ProjectPythia:main Jun 15, 2021
@brian-rose
Copy link
Member

Great work all! We're checking things off the milestone list...

@dcamron dcamron deleted the core-mpl branch June 16, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Content related issue ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants