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

Ncl Climatology Entry #80

Merged
merged 41 commits into from
Sep 26, 2024
Merged

Ncl Climatology Entry #80

merged 41 commits into from
Sep 26, 2024

Conversation

andy-theia
Copy link
Contributor

@andy-theia andy-theia commented Jul 16, 2024

PR Summary

  • Creates NCL entry for several climatology functions
  • Creates NCL receipt for several climatology functions

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 Jul 16, 2024

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

@kafitzgerald
Copy link
Collaborator

Just noting that pre-commit is failing because of image(s) again and nothing else.

@andy-theia
Copy link
Contributor Author

Just noting that pre-commit is failing because of image(s) again and nothing else.

Actually in this case I think it is failing because of

`month_to_season` computes a user-specified three-month seasonal mean (DJF, JFM, FMA, MAM, AMJ, MJJ, JJA, JAS, ASO, SON, OND, NDJ)

It doesn't seem to like OND very much

@kafitzgerald
Copy link
Collaborator

To address the "OND" issue (if you want), you could add in a pyproject.toml text file to the base directory of the repository with a configuration option to ignore that set of letters. You can see an example over on geocat-comp. You should only need the following lines though:

[tool.codespell]
ignore-words-list = "ond"

There's another way to do this with a .codespellrc file, but by adding in the pyproject.toml we should be able to keep more of the configuration of helper tooling in the same place rather than adding a config file for each.

@kafitzgerald kafitzgerald marked this pull request as ready for review September 24, 2024 22:54
@kafitzgerald kafitzgerald added the blocked Work blocked waiting on the output of some other source/work label Sep 25, 2024
@kafitzgerald kafitzgerald removed the blocked Work blocked waiting on the output of some other source/work label Sep 25, 2024
@@ -0,0 +1,313 @@
{
Copy link
Contributor

@cyschneck cyschneck Sep 26, 2024

Choose a reason for hiding this comment

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

The width of the plot is cutting off the title values


Reply via ReviewNB

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch.

I'll go in and add some custom titles that won't take up so much space.

I could probably add the semicolon after the plot as well to avoid the matplotlib lines above the figures as well.

Removing the figures altogether is another option (and would be more concise), but I think I'll leave that call for later.

@@ -0,0 +1,313 @@
{
Copy link
Contributor

@cyschneck cyschneck Sep 26, 2024

Choose a reason for hiding this comment

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

Also here, the width of the plot is cutting off the title values


Reply via ReviewNB

@@ -0,0 +1,313 @@
{
Copy link
Contributor

@cyschneck cyschneck Sep 26, 2024

Choose a reason for hiding this comment

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

Also here, the width of the plot is cutting off the title values


Reply via ReviewNB

@@ -9,8 +9,15 @@ NCL Function,Description,Python Equivalent,Notes
`cosh <https://www.ncl.ucar.edu/Document/Functions/Built-in/cosh.shtml>`__,"Computes the hyperbolic cosine of numeric types","``math.cosh()`` or ``numpy.cosh()``",`example notebook <../ncl_entries/trigonometric_functions.ipynb#cosh>`__
`sinh <https://www.ncl.ucar.edu/Document/Functions/Built-in/sinh.shtml>`__,"Computes the hyperbolic sine of numeric types","``math.sinh()`` or ``numpy.sinh()``",`example notebook <../ncl_entries/trigonometric_functions.ipynb#sinh>`__
`tanh <https://www.ncl.ucar.edu/Document/Functions/Built-in/tanh.shtml>`__,"Computes the hyperbolic tangent of numeric types","``math.tanh()`` or ``numpy.tanh()``",`example notebook <../ncl_entries/trigonometric_functions.ipynb#tanh>`__
`days_in_month <https://www.ncl.ucar.edu/Document/Functions/Built-in/days_in_month.shtml>`__,"Calculates the number of days in a month given month and year","``cftime.datetime().daysinmonth``",`example notebook <../ncl_entries/days_in_month.ipynb>`__
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be alphabetical? It looks like they're not rendered in the same order as this table anyway - but not quite alphabetical either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A similar thought process led me to logging this issue (better table functionality including sorting): #119

I was leaning toward leaving it for now and sorting things out later when addressing that issue, but could be convinced otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah separate PR for sure. Thanks for making that issue

@jukent jukent self-requested a review September 26, 2024 20:36
@kafitzgerald
Copy link
Collaborator

I think there's still some room to polish up the plots here, but I think that can happen later. Maybe as part of some project-wide formatting / style work.

I'm going to get this merged.

@kafitzgerald kafitzgerald merged commit 5f8f71f into NCAR:main Sep 26, 2024
2 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants