Skip to content

CMIP6 CanESM CMOR fixes for variables with model level coordinates#3165

Merged
rbeucher merged 14 commits into
mainfrom
canesm_cmor_fixes
Jul 13, 2026
Merged

CMIP6 CanESM CMOR fixes for variables with model level coordinates#3165
rbeucher merged 14 commits into
mainfrom
canesm_cmor_fixes

Conversation

@k-a-webb

@k-a-webb k-a-webb commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Description

For variables in the CMIP6 CanESM output which are published on model levels, the coordinate ps has the standard name air_pressure. This causes an issue as the derived pressure level coordinate also has the standard name air_pressure.

For example:

esmvalcore.cmor.check.CMORCheckError: There were errors in variable cli:
 There are multiple coordinates with standard_name "air_pressure": ['ps', None]

The new fix relabels the standard name of ps to surface_air_pressure.

Closes #3164

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:

…re, fix renames it surface_air_pressure, to be distinct from the derived coordinate for air pressure, which is given the standard name air_pressure
@CLAassistant

CLAassistant commented Jul 13, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@k-a-webb k-a-webb marked this pull request as draft July 13, 2026 18:41
@k-a-webb

Copy link
Copy Markdown
Contributor Author

@valeriupredoi Would you mind helping me resolve the failed commit fix? I'm lost as to how to handle this.

@k-a-webb k-a-webb marked this pull request as ready for review July 13, 2026 20:26
@k-a-webb k-a-webb requested a review from malininae July 13, 2026 21:45
@k-a-webb k-a-webb marked this pull request as draft July 13, 2026 21:45
@malininae

Copy link
Copy Markdown
Contributor

@k-a-webb and I looked at the code and we've figured out where the issue in tests coming from. Kristi will adjust accordingly and I can act as one of the reviewers.

@k-a-webb k-a-webb marked this pull request as ready for review July 13, 2026 22:14
Comment thread esmvalcore/cmor/_fixes/cmip6/canesm5.py
Comment thread tests/integration/cmor/_fixes/cmip6/test_canesm5.py Outdated
Comment thread esmvalcore/cmor/_fixes/cmip6/canesm5_1.py Outdated
Comment thread tests/integration/cmor/_fixes/cmip6/test_canesm5_1.py
@k-a-webb k-a-webb requested a review from malininae July 13, 2026 22:43

@malininae malininae left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me! I'd say it's ready for review from someone with write permissions.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.25%. Comparing base (c814e65) to head (71f92e2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3165   +/-   ##
=======================================
  Coverage   96.24%   96.25%           
=======================================
  Files         273      274    +1     
  Lines       16172    16189   +17     
=======================================
+ Hits        15565    15582   +17     
  Misses        607      607           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rbeucher

Copy link
Copy Markdown
Contributor

I don't understand the test logic; none of the classes in Canesm5_1.py are touched by the tests.

@rbeucher rbeucher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The tests are not touching the new classes definition in canesm5_1.py

@k-a-webb

Copy link
Copy Markdown
Contributor Author

The tests are not touching the new classes definition in canesm5_1.py

We were following the example of https://github.com/ESMValGroup/ESMValCore/blob/canesm_cmor_fixes/esmvalcore/cmor/_fixes/cmip6/mpi_esm1_2_xr.py, which calls all it's classes from mip_esm1_2_hr, and has no explicit test in https://github.com/ESMValGroup/ESMValCore/tree/canesm_cmor_fixes/tests/integration/cmor/_fixes/cmip6

That said, I can generate a near duplicate of test_canesm5.py if that is preferred.

@rbeucher

Copy link
Copy Markdown
Contributor

I think it's better to have a test_canesm5_1.py Thank you

@rbeucher rbeucher merged commit e08d0f8 into main Jul 13, 2026
4 checks passed
@rbeucher rbeucher deleted the canesm_cmor_fixes branch July 13, 2026 23:32
@flicj191 flicj191 mentioned this pull request Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dataset problem: CMIP6 CanESM CMOR fix needed for variables with model level coordinates

4 participants