Add day of week - Githubissues

NCAR / geocat-applications

GeoCAT Applications is a community resource inspired by the NCL Applications page.

https://ncar.github.io/geocat-applications/

Apache License 2.0

4 stars 5 forks source link

Add day of week #51

Closed andy-theia closed 3 weeks ago

andy-theia commented 1 month ago

PR Summary

Adds day_of_week in ncl/ncl_entries/day_of_week.ipynb
Updates the NCL Index to include day_of_week
Adds "Calculating the day of the week" section to applications/date_time/datetime.ipynb

Related Tickets & Documents

Closes #46

PR Checklist

General

[x] PR includes a summary of changes
[x] Link relevant issues, make one if none exist

review-notebook-app[bot] commented 1 month ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

github-actions[bot] commented 1 month ago

Meowdy! See your PR preview: 🔍 Git commit SHA: 11a58d9bf0498a51ef6a2588f989634ce69451ea ✅ Deployment Preview URL: https://NCAR.github.io/geocat-applications/_preview/51

andy-theia commented 4 weeks ago

Looking good, a minor edit:

The line:
However this can be adjusted for by adding has_year_zero=True.
Would be good to include as part of the Using cftime section since it is a reason to use cftime over datetime

Thanks Cora, I will play around with moving that bit into the Using cftime section. You are right that it might make more sense up there.

anissa111 commented 4 weeks ago

In addition to the notebook review, I have a few miscellaneous comments:

do we want to add a section to "specific use cases" under the python datetime.ipynb?
should we include a receipt with more extensive testing/verification?

@kafitzgerald

anissa111 commented 4 weeks ago

Also, let us know if you have any questions with merge conflicts now that #44 is merged!

kafitzgerald commented 4 weeks ago

In addition to the notebook review, I have a few miscellaneous comments:

do we want to add a section to "specific use cases" under the python datetime.ipynb?

should we include a receipt with more extensive testing/verification?

@kafitzgerald

I think I suggested holding off on these (for now) until the template and structure for those was more stable. And maybe this was why we changed this PR description to "relates to" rather than "closes" the issue.

Those could be a good next steps though and admittedly this order of NCL to Python content followed by a receipt and Python first content is maybe not something we should generally encourage.

kafitzgerald commented 3 weeks ago

Looking good, a minor edit:

The line:
However this can be adjusted for by adding has_year_zero=True.
Would be good to include as part of the Using cftime section since it is a reason to use cftime over datetime

@cyschneck do the changes address your concerns? I think the re-review request might've gotten lost last week in the pile of updates.