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.

Sign up for GitHub

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

Jump to bottom

Adding key metrics: Isla's NMSE metric #121

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Adding key metrics: Isla's NMSE metric #121

merged 7 commits into from
Aug 7, 2024

Conversation

TeaganKing
Copy link
Collaborator

@TeaganKing TeaganKing commented Aug 5, 2024 •
edited
Loading

This PR brings in the NMSE key metrics which @islasimpson created for atm.

This is a useful example of a key metric that could also be used in the diagnostics portion of the CESM tutorial.

--

All Submissions:

  • Have you followed the guidelines in our Contributor's Guide (including the pre-commit check)?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Have you lint your code locally prior to submission?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully ran tests with your changes locally?

@TeaganKing TeaganKing self-assigned this Aug 5, 2024
mnlevy1981
mnlevy1981 requested changes Aug 5, 2024
View reviewed changes
Copy link
Collaborator

@mnlevy1981 mnlevy1981 left a comment

Choose a reason for hiding this comment

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

I'll wait until you've had a chance to do your final testing before running the notebook myself, though a few cells caught my eye

examples/key_metrics/config.yml Outdated Show resolved Hide resolved
examples/key_metrics/config.yml Outdated Show resolved Hide resolved
examples/nblibrary/atm/averaging_utils.py Outdated Show resolved Hide resolved
examples/nblibrary/atm/nmse_PSL.ipynb Outdated Show resolved Hide resolved
examples/nblibrary/atm/nmse_PSL.ipynb Outdated Show resolved Hide resolved
@TeaganKing TeaganKing requested a review from mnlevy1981 August 7, 2024 02:58
@TeaganKing
Copy link
Collaborator Author

Screen Shot 2024-08-06 at 8 51 55 PM This is now generating plots as expected with `cupid-run`! @mnlevy1981 this is ready for your re-review.

@mnlevy1981
Refactor nmse_PSL to work with CESM2 f19 output
a38b0b0 
1. To work with CESM2 output, needed to include a fix_time_dim() function that
   sets time to the averaging midpoint rather than the endpoint.
2. Output that has been been regridded in post-processing is in proc/tseries/
   rather than proc/tseries/regrid/, so user needs to control that from
   config.yml
3. Runs on the fv1.9x2.5 grid need to compare against coarser observational
   data (so the location of validation data is set in config.yml)
@mnlevy1981
Copy link
Collaborator

@TeaganKing can you do a git pull and make sure a38b0b0 works for you? If it does, I think this is ready to merge :)

@mnlevy1981 mnlevy1981 marked this pull request as ready for review August 7, 2024 21:01
@mnlevy1981 mnlevy1981 mentioned this pull request Aug 7, 2024
Merged
@TeaganKing
Copy link
Collaborator Author

This all runs smoothly and looks good to me! Thanks for your work on this, Mike and Isla! Merging in now.

@TeaganKing TeaganKing merged commit d2cc4d0 into NCAR:main Aug 7, 2024
1 check passed
@TeaganKing TeaganKing deleted the isla_nmse branch November 21, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@islasimpson islasimpson islasimpson left review comments

@kafitzgerald kafitzgerald kafitzgerald left review comments

@mnlevy1981 mnlevy1981 mnlevy1981 approved these changes

Assignees

@TeaganKing TeaganKing

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TeaganKing @mnlevy1981 @kafitzgerald @islasimpson