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

Fixing missing height 2m coordinates in GFDL-CM4 and KIOST-ESM #2294

Merged
merged 14 commits into from
Jan 16, 2024

Conversation

Karen-A-Garcia
Copy link
Contributor

@Karen-A-Garcia Karen-A-Garcia commented Jan 3, 2024

This is a follow up to issues #2293 and #2295. This is all using daily resolution CMIP6 data. In GFDL-CM4 in historical experiment tasmax, tasmin, sfcWind, and hurs are missing the 2m height coordinate. In Kiost-esm tasmin and tasmax has height issues. I just added quick fixes for those and tests for them. This is similar to what I did in pull request #2263.

@Karen-A-Garcia Karen-A-Garcia changed the title Fixing missing height 2m coordinates in GFDL-CM4 Fixing missing height 2m coordinates in GFDL-CM4 and KIOST-ESM Jan 4, 2024
@Karen-A-Garcia Karen-A-Garcia added bug Something isn't working fix for dataset Related to dataset-specific fix files labels Jan 4, 2024
Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f515169) 93.61% compared to head (97bb450) 93.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2294      +/-   ##
==========================================
+ Coverage   93.61%   93.70%   +0.09%     
==========================================
  Files         238      238              
  Lines       13113    13115       +2     
==========================================
+ Hits        12276    12290      +14     
+ Misses        837      825      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@valeriupredoi
Copy link
Contributor

valeriupredoi commented Jan 16, 2024

hi @Karen-A-Garcia happy new year and many thanks for the fixes and PR! I fixed the conflict in the test module, and will merge after them tests pass. A couple small pointers: when you import classes or functions from module, it's best practice to import them all in one line, rather than eg from moduleA import class1 / from moduleA import class2 do from moduleA import (class1, class2, ). Also, once you see merge conflicts, it's always good to, given you got time, fix them, or if the conflicts are something you don't really know how to fix, ask for help. One last thing - please work on branches that have different names for different things, it's very confusing (and sometimes dangerous) to recycle branch names (once they've been merged and deleted). Cheers 🍺

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

great, cheers! Nevermind my rant about imports (above) - saw this was due to the age of the branch, rather than of your own making 😁

@valeriupredoi valeriupredoi merged commit 79455c1 into main Jan 16, 2024
5 checks passed
@valeriupredoi valeriupredoi deleted the fixes_mpi_highresmip_models branch January 16, 2024 14:33
@Karen-A-Garcia
Copy link
Contributor Author

Still good advice! Thanks @valeriupredoi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants