Missing 2m height coordinate and monotonicity for tasmin in CESM2 and CESM2-WACCM

[Karen-A-Garcia]

This is a follow up to issue #2573 for the height coordinates. Still looking into the time issue. There was already a fix for the 2m height previously done for tas and I simply applied the fix to tasmin in both CESM2 and CESM2-WACCM files. There were already tests written for the fixes so I didn't add anything to those files.

• ☝ Create an issue to discuss what you are going to do

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

• 🧪 The new functionality is relevant and scientifically sound
• 🛠 This pull request has a descriptive title and labels
• 🛠 Code is written according to the code quality guidelines
• 🧪 and 🛠 Documentation is available
• 🛠 Unit tests have been added
• 🛠 Changes are backward compatible
• 🛠 Any changed dependencies have been added or removed correctly
• 🛠 The list of authors is up to date
• 🛠 All checks below this pull request were successful

[codecov]

Codecov Report

Attention: Patch coverage is 97.18310% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.68%. Comparing base (1cf3862) to head (c0e5236).
Report is 52 commits behind head on main.

Files with missing lines	Patch %	Lines
esmvalcore/cmor/_fixes/cmip6/cesm2.py	96.72%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2574      +/-   ##
==========================================
+ Coverage   94.66%   94.68%   +0.01%     
==========================================
  Files         251      251              
  Lines       14302    14372      +70     
==========================================
+ Hits        13539    13608      +69     
- Misses        763      764       +1     

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

[valeriupredoi]

looks good, many thanks! Would you mind running pre-commit before hand please? Also, will approve and merge if you don't want to plug in the monotonicity fix here as well 🍺

[Karen-A-Garcia]

Thanks @valeriupredoi and thanks for showing me a similar fix for the monotonicity too. I've added a fix for the monotonicity as well as adding tests for them and I ran pre-commit before sending pushing the changes. Let me know if there's anything else I need to change.

[valeriupredoi]

many thanks @Karen-A-Garcia 🍺 A few wiggles to fix (am not 100% sure of the baseclass for Tasmin for CESM2, I think it should be a generic Fix class there, doesn't really matter that much). One general comment related to testing: it's great that you added tests, this is always a good thing to see from users, but wrt the testing framework, I'd suggest using the Pytest test classes/functions, and not use Unittest test classes anymore - these are old school and a tad harder to adapt inside a Pytest testing framework, plus, they need a lot more indentation 😁

[Karen-A-Garcia]

Thank you @valeriupredoi for looking over my code.🍺 I've changed all the old test formats and changed them so that they run with pytest. I had no issues on my end with the tests when I ran them in my terminal but let me know if there's anything else I need to change. Also looks like CESM2-FV2 and CESM2-WACCM-FV2 had some monotonicity issue so I've import changes to Tas and Pr made in CESM2 into those files. There was also one test that failed from some code that was already in the file and I couldn't really figure out how to fix it so I left it as it was.

[valeriupredoi]

hi @Karen-A-Garcia - these tests have not been corrected: they are still in UnitTest format, but a bit more critical, are not testing for the correct model, and also have issues with the actual tested outcome vs expected, could you please have another look at them? My code suggestion change appears to have been resolved, but the model is still wrong: you should test for CESM2-WACCM, not NCAR. Perhaps you forgot to push the changes? 🍺

[Karen-A-Garcia]

Oh that is so strange because it was changed on my files when I pushed them and they seemed to not have change for some reason. I'll try to fix that.

[valeriupredoi]

No worries at all! I was suspecting it's a push issue. If it's too much trouble to convert the tests to simple pytest funcs, don't worry about it, it's great you plopped tests in anyways. Let me know if I can help in any way 👍🍺

[valeriupredoi]

many thanks @Karen-A-Garcia ! There are still some issues, unfortunately, please see the failed tests, and my comment above, perhaps you forgot to push to remote? Cheers! PS: I turned on Github Actions tests, since it's a lot easier to see test fails there than on CircleCI, see eg test run output

[Karen-A-Garcia]

@valeriupredoi Okay I think I've changed all the UnitTest formats into pytest now and the failed tests are coming from that code that I was previously talking about that was failing when I ran it in my terminal as well.

[valeriupredoi]

excellent work @Karen-A-Garcia - many thanks! All spiffy now: the issue was that the Tas() fix was returning both the original cubes and the newly fixed cubes in the new_cubes iris CubeList(), and that the test was comparing a Python list with an iris CubeList() construct. Fixed that and it's ready to be merged when all tests pass 🍺

[valeriupredoi]

very many thanks @Karen-A-Garcia 🍻