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

daylight_fao56 #115

Merged
merged 17 commits into from
Oct 14, 2024
Merged

daylight_fao56 #115

merged 17 commits into from
Oct 14, 2024

Conversation

cyschneck
Copy link
Contributor

PR Summary

Add Meteorology max_daylight geocat function to resolve NCL daylight_fao56 function

Covers:

Related Tickets & Documents

Closes #104

PR Checklist

General

  • PR includes a summary of changes
  • Link relevant issues, make one if none exist
  • If adding a new page, select which type:
    • NCL Entry
    • Receipt

@cyschneck cyschneck self-assigned this Sep 6, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

github-actions bot commented Sep 6, 2024

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

@cyschneck cyschneck added ncl receipt Receipt files are small files with little to no narrative content ncl entry NCL entries are pages that explain specifically how to achieve something that was possible in NCL labels Oct 10, 2024
@cyschneck
Copy link
Contributor Author

Combined dewtemp and daylight functions into a single NCL entry, which will eventually include additional meteorology functions

@cyschneck cyschneck marked this pull request as ready for review October 10, 2024 19:06
@jukent
Copy link
Collaborator

jukent commented Oct 10, 2024

@cyschneck I won't be able to look at this until Monday.

Copy link
Collaborator

@kafitzgerald kafitzgerald left a comment

Choose a reason for hiding this comment

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

A few suggestions, but overall this looks good.

The receipt is a little hard to read, but I think a good bit of that is from the formatting which is probably inherited from other receipts and refactoring that is beyond the scope of this PR.

ncl/ncl_entries/meteorology.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/meteorology.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/meteorology.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/meteorology.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/meteorology.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/meteorology.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/meteorology.ipynb Outdated Show resolved Hide resolved
ncl/ncl_entries/meteorology.ipynb Outdated Show resolved Hide resolved
ncl/receipts/meteorology.ipynb Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Approved, pending @kafitzgerald 's suggestions.

I want to make sure we're remembering all of the formatting and other comments we've made in PRs that are outside the scope of those PRs

cyschneck and others added 5 commits October 14, 2024 14:38
Co-authored-by: Katelyn FitzGerald <7872563+kafitzgerald@users.noreply.github.com>
Co-authored-by: Katelyn FitzGerald <7872563+kafitzgerald@users.noreply.github.com>
Co-authored-by: Katelyn FitzGerald <7872563+kafitzgerald@users.noreply.github.com>
Co-authored-by: Katelyn FitzGerald <7872563+kafitzgerald@users.noreply.github.com>
@kafitzgerald
Copy link
Collaborator

The pre-commit failure here looks like it's the same as on NCAR/geocat-examples#618.

I ended up just commenting docformatter out for now in the pre-commit config and logging an issue to add it back once the new release is out to address this there.

@cyschneck
Copy link
Contributor Author

cyschneck commented Oct 14, 2024

The pre-commit failure here looks like it's the same as on NCAR/geocat-examples#618.

I ended up just commenting docformatter out for now in the pre-commit config and logging an issue to add it back once the new release is out to address this there.

Will do

@cyschneck cyschneck merged commit 118cb1b into NCAR:main Oct 14, 2024
2 checks passed
@cyschneck cyschneck deleted the daylight_104 branch October 14, 2024 21:19
github-actions bot pushed a commit that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ncl entry NCL entries are pages that explain specifically how to achieve something that was possible in NCL ncl receipt Receipt files are small files with little to no narrative content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GA-1516] Meteorology - daylight_fao56
3 participants