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

Drop in Unidata datetime intro materials notebook #38

Merged
merged 20 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 some of Unidata's introductory python materials that lives alongside our workshop materials. Mostly content-unmodified.

There were some headaches getting this to play nicely with jupyter-book including some hidden rogue widget and name metadata that totally broke the process. Mentioning that for myself and anyone else that might run into similar issues, as the tracebacks weren't particularly helpful.

Notes:

  • In my opinion, one of the better notebooks I've dropped in so far with regards to narrative and guiding text
  • The scope might be a bit of a mixed bag. Defining classes/functions and using the mesowest API to discuss datetime
  • Might be a bit too tightly dependent on scientific contexts, but does have varied examples
  • Organization fits current theme pretty well. Might benefit from more consistent titling as-is

@dcamron dcamron added the content Content related issue label Apr 20, 2021
@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 276c1d9 at: https://607f5a578f8ed000dc5259a9--pythia-foundations.netlify.app

@clyne clyne added this to the InitialPortalRelease milestone Jun 4, 2021
@brian-rose
Copy link
Member

I'll work on massaging this to the template during today's hackathon.

@brian-rose brian-rose 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

@brian-rose
Copy link
Member

Ok, I've done my best to adapt this to the template.

A few issues that I see:

  • The linter is getting in the way! Specifically, our linters impose strict requirements on import statement (e.g. alphabetical ordering). It's an issue here because part of the tutorial involves an explicit discussion about the need to alias the imports with e.g import datetime as dt to disambiguate names.
  • The Unidata notebook contained an explicit link to another notebook on formatted input/output. I wasn't sure what to do with that since there is no equivalent tutorial in our Foundations collection.
  • The side-trip into OOP and Python classes is cool! But does it belong here? Would it make more sense to break this out into a free-standing tutorial? Or put it somewhere earlier in the Foundations sequence? I'm not sure.
  • To my taste, the METAR parsing example is just too complicated and too domain-specific. It will get a novice learner bogged down in too many details, e.g. about URL formatting and JSON files. Let's hear other opinions about this!

@github-actions
Copy link

github-actions bot commented Jun 9, 2021

🚀 📚 Preview for git commit SHA: 44088ff at: https://60c10a6e17a1510de2fcdc54--pythia-foundations.netlify.app

@github-actions
Copy link

github-actions bot commented Jun 9, 2021

🚀 📚 Preview for git commit SHA: 566d7f6 at: https://60c10d5b8923870f2fc17b52--pythia-foundations.netlify.app

@dopplershift
Copy link
Contributor

I agree about METAR and the function. Just too many things going on. Honestly, I don't even remember when that got added.

@github-actions
Copy link

github-actions bot commented Jun 9, 2021

🚀 📚 Preview for git commit SHA: 10e2bbb at: https://60c1128abdc7741a41fce2fa--pythia-foundations.netlify.app

core/datetime/datetime.ipynb Show resolved Hide resolved
core/datetime/datetime.ipynb Show resolved Hide resolved
core/datetime/datetime.ipynb Show resolved Hide resolved
core/datetime/datetime.ipynb Show resolved Hide resolved
core/datetime/datetime.ipynb Show resolved Hide resolved
core/datetime/datetime.ipynb Show resolved Hide resolved
@clyne
Copy link
Contributor

clyne commented Jun 9, 2021

Looks great, @dcamron. I just had a few minor suggestions.

@brian-rose
Copy link
Member

Looks great, @dcamron. I just had a few minor suggestions.

Since I've sort of taken over responsibility for this PR, I responded to your comments @clyne

@brian-rose
Copy link
Member

I agree about METAR and the function. Just too many things going on. Honestly, I don't even remember when that got added.

Based on this comment and my own fairly strong feelings about this, I went ahead and removed this section. It leaves behind a cleaner, quicker introductory tutorial.

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 153282e at: https://60c27af5a378a900b8dfc78e--pythia-foundations.netlify.app

@ktyle
Copy link
Contributor

ktyle commented Jun 10, 2021

This is looking great!

With regard to the inclusion of the "30-second intro to OOP", my sense is that this ultimately belongs elsewhere in the Foundations content, ideally as a later section of the Intro to Python chapter. But it's really good! How about we include it as is in this notebook for now, and later on we can replace it in this notebook with a "reminder about OOP as it relates to datetime" link to where it will ultimately go?

@clyne clyne self-requested a review June 10, 2021 21:58
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.

Looks great!

@brian-rose
Copy link
Member

This is looking great!

With regard to the inclusion of the "30-second intro to OOP", my sense is that this ultimately belongs elsewhere in the Foundations content, ideally as a later section of the Intro to Python chapter. But it's really good! How about we include it as is in this notebook for now, and later on we can replace it in this notebook with a "reminder about OOP as it relates to datetime" link to where it will ultimately go?

I will open an issue about this when we merge this PR so we don't forget about it entirely.

core/datetime/datetime.ipynb Show resolved Hide resolved
@@ -0,0 +1,617 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of directly using strftime, would it be easier for the learner to instead do something like:

print(f'{strike_time: %Hh %Mm %Ss}') for symmetry with the other string formatting we're using?


Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

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

Wow, I'm actually surprised that that works?

Like, to me it's not intuitive that passing strike_time as an f-string would let you use things like %H and have it be interpreted properly.

But then, I only learned about f-strings a few weeks ago!

As far as symmetry, I think the existing content goes for symmetry between strftime() and strptime() .

Copy link
Contributor

@dopplershift dopplershift Jun 15, 2021

Choose a reason for hiding this comment

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

Well, an fstring is just syntactic sugar for format, which uses an object's __format__; for a datetime object, __format__ uses the same code as strftime.

from datetime import datetime
dt = datetime.now()

# All the same
print(f'{dt:%Y%m%d}')
print('{0:%Y%m%d}'.format(dt))
print(dt.strftime('%Y%m%d'))

It's nice because the %Y, etc. work the same way as you would '{:6.2f}'.format(1.23) to format a floating point value.

I'd honestly argue that strftime is more a relic of the C way of doing things and using f''/format is the Pythonic way. But I also find the strptime and strftime pair convincing, so I'm not really arguing 😉. It's fine to leave as is.

core/datetime/datetime.ipynb Show resolved Hide resolved
@@ -0,0 +1,617 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be good to point out that timezone.utc is now a thing that exists in the standard library? Basically, you now have a UTC (and only UTC) timezone in the standard library. This was added since the original material was created.


Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks! I will add a little more elaboration here. I think the original was a bit mysterious about why you need a third-party library here.

@clyne clyne requested a review from anissa111 June 14, 2021 20:57
@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 6e89dd5 at: https://60c7e31453caca3751ab671e--pythia-foundations.netlify.app

@brian-rose
Copy link
Member

Ha ha, apparently GitHub Actions works in UTC, which kind of wrecks my new example :-(

Copy link
Member

@anissa111 anissa111 left a comment

Choose a reason for hiding this comment

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

I agree with earlier comments that the "Into to OO" ultimately should be moved elsewhere, but it looks like there are already plans to address this in the future.

This notebook looks great to me!

@brian-rose
Copy link
Member

I added some extra explanation to the UTC example, since it looked funny after executing on the server where local time = UTC.

I'll merge this tomorrow in case @dopplershift wants to respond to any of the edits.

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 0c70084 at: https://60c815986aeb136b361930d5--pythia-foundations.netlify.app

@dopplershift dopplershift merged commit 03f0e34 into ProjectPythia:main Jun 15, 2021
@dcamron dcamron deleted the core-datetime 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