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

on-the-fly cmoriser for ACCESS native data #2430

Merged
merged 130 commits into from
Jul 9, 2024

Conversation

rhaegar325
Copy link
Contributor

@rhaegar325 rhaegar325 commented May 21, 2024

Description

This pull request is to add the first version of on-the-fly cmoriser for ACCESS native data.
in this version we support only two variables(tas, pr), and will try to develop a more general version in next version.

Closes ESMValGroup/ESMValTool#3431

Link to documentation:


Before you get started

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:

@rhaegar325
Copy link
Contributor Author

@rbeucher our on-the-fly cmoriser pr is there, fixing issues to pass those tests

environment.yml Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
@rbeucher
Copy link
Contributor

Thanks Rhaegar. I think we need to update the documentation too. I will have a go.

Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 36.53846% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 93.85%. Comparing base (f72a873) to head (0e864b6).
Report is 39 commits behind head on main.

Files Patch % Lines
esmvalcore/cmor/_fixes/access/access_esm.py 36.53% 33 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2430      +/-   ##
==========================================
- Coverage   94.07%   93.85%   -0.23%     
==========================================
  Files         241      242       +1     
  Lines       13422    13474      +52     
==========================================
+ Hits        12627    12646      +19     
- Misses        795      828      +33     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 94.64%. Comparing base (1da5bbc) to head (987bdd9).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2430      +/-   ##
==========================================
+ Coverage   94.61%   94.64%   +0.02%     
==========================================
  Files         246      248       +2     
  Lines       14048    14105      +57     
==========================================
+ Hits        13292    13349      +57     
  Misses        756      756              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

Yousong Zeng and others added 4 commits May 24, 2024 14:23
@rbeucher
Copy link
Contributor

Hi @rhaegar325 ,

See the PR here #1678 for CESM2
I think you need to add some tests and also make sure that the general structure, including the mapping information, is in the correct location.

Copy link
Contributor

@rbeucher rbeucher left a comment

Choose a reason for hiding this comment

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

Hi Rhaegar.

It looks cleaner. However, specific variable fixes should be defined in specific classes. AllVar is supposed to be general and shouldn't have conditional sections based on the variable. See @schlunma comments.

esmvalcore/cmor/_fixes/access/access_esm.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/access/access_esm1_5.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/access/access_esm1_5.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rbeucher rbeucher left a comment

Choose a reason for hiding this comment

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

Just a few comments. I think it is good to go. I am not sure about the sub_dataset facet name... @flicj191 any ideas?

Comment on lines +621 to +623
``sub_dataset`` Part of the ACCESS-ESM raw dataset No default
root, need to specify if you want to
use the cmoriser
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it part of the root ? I don't think so. careful to not create confusion with the rootpath in the config file.

class Rlus(AccessFix):
"""Fixes for Rlus."""

def fix_rlus_data(self, cubes):
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to fix_data

class Rsus(AccessFix):
"""Fixes for Rsus."""

def fix_rsus_data(self, cubes):
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to fix_data

@rbeucher
Copy link
Contributor

rbeucher commented Jul 8, 2024

You need to fix the tests @rhaegar325

@rbeucher rbeucher dismissed schlunma’s stale review July 9, 2024 01:18

Just bypassing this

@rbeucher rbeucher merged commit 6b4e3fb into ESMValGroup:main Jul 9, 2024
3 of 4 checks passed
@rbeucher rbeucher mentioned this pull request Jul 9, 2024
@schlunma
Copy link
Contributor

schlunma commented Jul 9, 2024

Hi @rbeucher, sorry, since there were so many commits on this lately I was waiting for a comment saying that it's ready for a final check. I would have loved to take one more look at this.

Before dismissing someone's review, please ask them and give them some time. If you don't get an answer after that, it's of course fine to dismiss the review. As I said, I wasn't aware that this is ready for another review. Not a big problem of course, just wanted to make you aware of that!

Thanks @rhaegar325 and everyone for all the work on this, it's great to have support for native output of one more model πŸš€

@rbeucher
Copy link
Contributor

rbeucher commented Jul 9, 2024

Sorry @schlunma. I just had to make an executive decision on this one. We are preparing for a training event and we need to get things rolling. Thanks for reviewing this and sorry again.

rbeucher added a commit to ACCESS-NRI/ESMValCore_ACCESS_dev that referenced this pull request Jul 10, 2024
Co-authored-by: Yousong Zeng <Yousong>
Co-authored-by: Romain Beucher <romain.beucher@anu.edu.au>
Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>
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.

Build a new live-CMORiser for ACCESS-ESM raw data in ESMValTool
8 participants