Skip to content

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 and branding removed. Otherwise has been mostly content-unmodified.

Notes:

  • Fits pretty well on the current sphinx book theme, titles and images line up alright if inconsistent with some other notebooks so far
  • Reference choices?
  • Definitely needs tied into a larger unit on data formats, and will likely benefit with some further crossover with other sections later

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 9b5e13d at: https://607f4bca71ba8900a2044bbd--pythia-foundations.netlify.app

@dcamron dcamron added the content Content related issue label Apr 20, 2021
@@ -0,0 +1,901 @@
{
Copy link
Contributor

@mgrover1 mgrover1 May 5, 2021

Choose a reason for hiding this comment

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

This all looks fantastic - I think it would be good to show an example of cf-xarray and why it it can be useful to follow this/how it can make operations more general when you can use this metadata


Reply via ReviewNB

@clyne clyne added this to the InitialPortalRelease milestone Jun 4, 2021
@ktyle ktyle added the hackathon Issue highlighted for active hackathon session label Jun 9, 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: 4945a75 at: https://60c248c97a039e125de11429--pythia-foundations.netlify.app

@ktyle ktyle requested review from a team, dopplershift and jukent and removed request for a team June 10, 2021 17:21
@ktyle
Copy link
Contributor

ktyle commented Jun 10, 2021

This has been fit to the template and is ready for review.

" :comment = \"\" ;\n",
"}\n",
"```\n",
"We can also add attributes to this variable to define metadata. The CF conventions require a `units` attribute to be set for all variables that represent a dimensional quantity. The value of this attribute needs to be parsable by the UDUNITS library. Here we set it to a value of `'Kelvin'`. We also set the standard (optional) attributes of `long_name` and `standard_name`. The former contains a longer description of the variable, while the latter comes from a controlled vocabulary in the CF conventions. This allows users of data to understand, in a standard fashion, what a variable represents. If we had missing values, we could also set the `missing_value` attribute to an appropriate value."
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should say more about the UDUNITS library - a link to it at least.

Copy link
Contributor

@jukent jukent left a comment

Choose a reason for hiding this comment

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

Great work! I just have 2 comments/questions.

"cell_type": "markdown",
"metadata": {},
"source": [
"## Summary"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fill this section?

@clyne
Copy link
Contributor

clyne commented Jun 14, 2021

@ktyle just checking in to see if this is still doable for Wednesday. Thanks!

@ktyle
Copy link
Contributor

ktyle commented Jun 14, 2021

Should we defer to @dcamron to incorporate the items that have arisen from the reviews?

@ktyle
Copy link
Contributor

ktyle commented Jun 14, 2021

After discussion on the Zoom call, I will take care of incorporating the suggested edits (including my own) and will follow-up once it's ready for another review.

@clyne
Copy link
Contributor

clyne commented Jun 14, 2021

Just wanted to add that we had some confusion as to who currently owns this PR. We thought that it was @ktyle . Just want to cc @dcamron to give him the opportunity to chime in.

@ktyle
Copy link
Contributor

ktyle commented Jun 15, 2021

I'm in transit to Minneapolis this afternoon, but I will attend to @jukent 's review comments and push for final review by midnight this evening.

@clyne
Copy link
Contributor

clyne commented Jun 15, 2021

I'm in transit to Minneapolis this afternoon, but I will attend to @jukent 's review comments and push for final review by midnight this evening.

@ktyle give a shout if you want to hand this off. We'll figure it out.

@ktyle
Copy link
Contributor

ktyle commented Jun 15, 2021

I have pushed the notebook with content that responds to @jukent 's suggestions, as well as some other edits. The "Summary" and "What's Next" sections could probably be improved, but better than nothing.

Git reported that in addition to the netcdf-cf.ipynb file, two other files had changed:

(pythia-book-dev) ktyle@lore:~/html/pythia/foundations$ git status
On branch core-dataformats
Changes to be committed:
(use "git restore --staged ..." to unstage)
modified: core/data-formats/netcdf-cf.ipynb
new file: core/pandas/nino_analyzed_output.csv
modified: preamble/template.ipynb

I'm not sure how best to resolve those latter two files ... the netcdf-cf notebook is the only relevant change. I'm hoping someone else can take care of resolving and ultimately merging. Other than that, this is ready for another review.

@clyne
Copy link
Contributor

clyne commented Jun 15, 2021

I can try to sort out the conflicts. @jukent, @mgrover1 can you approve?

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 7333ffe at: https://60c93c95a47d1e5dfc6f49bc--pythia-foundations.netlify.app

Copy link
Contributor

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Looks great! Ready to merge!

@brian-rose brian-rose merged commit 1334e0e into ProjectPythia:main Jun 16, 2021
@dcamron dcamron deleted the core-dataformats 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 hackathon Issue highlighted for active hackathon session ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants