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

Added CESM2 CMORizer #1678

Merged
merged 15 commits into from
Oct 3, 2022
Merged

Added CESM2 CMORizer #1678

merged 15 commits into from
Oct 3, 2022

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Jul 27, 2022

Description

This PR implements a prototype of the on-the-fly CMORizer for the CESM2 model.

Link to documentation: https://esmvaltool--1678.org.readthedocs.build/projects/ESMValCore/en/1678/quickstart/find_data.html#cesm

Depends on #1718


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.


To help with the number pull requests:

@schlunma schlunma added the enhancement New feature or request label Jul 27, 2022
@schlunma schlunma added this to the v2.7.0 milestone Jul 27, 2022
@schlunma schlunma self-assigned this Jul 27, 2022
@schlunma schlunma changed the title First prototype of CESM2 on-the-fly CMORizer Added CESM2 CMORizer Jul 27, 2022
@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #1678 (b2af170) into main (4bb3061) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1678      +/-   ##
==========================================
+ Coverage   91.47%   91.48%   +0.01%     
==========================================
  Files         206      207       +1     
  Lines       11204    11218      +14     
==========================================
+ Hits        10249    10263      +14     
  Misses        955      955              
Impacted Files Coverage Δ
esmvalcore/cmor/_fixes/cesm/cesm2.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@schlunma schlunma marked this pull request as ready for review September 12, 2022 11:01
Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @schlunma, nice to have another model supported! This PR looks fine to me. Reusing shared fixes for native model output significantly reduces the number of code lines. I have a couple of minor suggestions, feel free to have a look. In particular, I would suggest to slightly expand the documentation to clarify which variables are currently supported and maybe how support could be added in the future for other cases.

tests/integration/cmor/_fixes/cesm/test_cesm2.py Outdated Show resolved Hide resolved
doc/quickstart/find_data.rst Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/cesm/cesm2.py Show resolved Hide resolved
@schlunma
Copy link
Contributor Author

Thanks for the review @remi-kazeroni! I expanded the doc and addressed all your comments. Let me know if you need anything else! 👍

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @schlunma! This looks good to me as it is 👍

@valeriupredoi
Copy link
Contributor

fellas @schlunma @remi-kazeroni I am ready to merge this if there are no major objections so I can include it in the 2.7 freezer ☃️ Please yell at me if otherwise, if not I will merge it by the end of day today 👍

@valeriupredoi
Copy link
Contributor

OK fellas, she's going in 🚢

@valeriupredoi valeriupredoi merged commit b5a8808 into main Oct 3, 2022
@valeriupredoi valeriupredoi deleted the cmorize_cesm branch October 3, 2022 14:45
@schlunma
Copy link
Contributor Author

schlunma commented Oct 4, 2022

Thanks V! Sorry for the late reply, we had a public holiday yesterday 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants