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 coordinates in MPI-ESM1-2-HR and MPI-ESM1-2-XR #2263

Merged
merged 11 commits into from
Dec 13, 2023

Conversation

Karen-A-Garcia
Copy link
Contributor

This is a follow up pull request for the issue #2262. This is all using daily resolution CMIP6 data. In MPI_ESM1_2_HR in HighResMIP, tasmax was missing the 2m height coordinate and uas and vas were missing the 10m height coordinate. In MPI_ESM1_2_XR in HighResMIP, tasmax and tasmin were missing the 2m height coordinate and uas and vas were missing the 10m height coordinate. I just added quick fixes for those.

@Karen-A-Garcia Karen-A-Garcia added fix for dataset Related to dataset-specific fix files data issue labels Nov 27, 2023
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.

Hi Karen, and welcome to contributing to ESMValCore! Very many thanks for your PR! Let's get started by looking at the boring bits first, so I can do a thorough review afterwards, when tests pass and all - could you please have a look at the failed flake8 test - you can run flake8 yourself in the ESMValCore dir by invoking (surprise, surprise) flake8 at command line 😁 These bits need attention:

(esmvaltool) valeriu@valeriu-PORTEGE-Z30-C:~/ESMValCore$ flake8 
./esmvalcore/cmor/_fixes/cmip6/mpi_esm1_2_hr.py:50:19: W291 trailing whitespace
./esmvalcore/cmor/_fixes/cmip6/mpi_esm1_2_hr.py:115:1: W293 blank line contains whitespace
./esmvalcore/cmor/_fixes/cmip6/mpi_esm1_2_hr.py:115:5: W292 no newline at end of file
./esmvalcore/cmor/_fixes/cmip6/mpi_esm1_2_xr.py:49:1: W391 blank line at end of file

After that, it'd be very nice and helpful if you could add tests for these new fixes - have a look at the test modules that are already there for these datasets eg the one for MPI ESM1-HR - let me know if you'd need some help with adding tests! Lastly, Codacy (style and standards) will have to be appeased, but let's do that at the end - cheers muchly for your work 🍺

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1cf02d3) 93.48% compared to head (03d3c98) 93.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2263      +/-   ##
==========================================
+ Coverage   93.48%   93.53%   +0.04%     
==========================================
  Files         238      238              
  Lines       12948    12955       +7     
==========================================
+ Hits        12105    12118      +13     
+ Misses        843      837       -6     

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

@Karen-A-Garcia
Copy link
Contributor Author

Hi @valeriupredoi I have changed all the flake8 issues for both mpi_esm1_2_hr.py and mpi_esm1_2_xr.py. I have also written some tests in test_mpi_esm1_2_hr.py. I have run it through pytest and it seems like the code has passed the tests.

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.

this is great, very many thanks @Karen-A-Garcia, much appreciated 🍺

@valeriupredoi valeriupredoi merged commit 3b37706 into main Dec 13, 2023
3 of 4 checks passed
@valeriupredoi valeriupredoi deleted the fixes_mpi_highresmip_models branch December 13, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data issue fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants