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

Removed scalar coordinates p0 and ptop prior to merge in multi_model_statistics #1471

Merged
merged 7 commits into from
Feb 8, 2022

Conversation

axel-lauer
Copy link
Contributor

@axel-lauer axel-lauer commented Feb 4, 2022

This is a solution for the issue described in #1222.

Description

Variables on hybrid vertical levels (e.g. clw) have to be converted to pressure or height levels to be able to be processed by some diagnostics and to be able to calculate meaningful multi-model statistics across different models. For this conversion, some models provide an auxiliary coordinate 'p0'. Calculating the multi-model mean over such datasets converted to pressure or height levels fails because 'p0' is kept after converting the vertical levels but (as expected) not identical in all models. This problem is solved by deleting the scalar auxiliary coordinates p0 and ptop (if present) after converting to pressure or altitude levels.

Closes #1222

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:

@axel-lauer axel-lauer requested a review from schlunma February 4, 2022 13:46
@axel-lauer axel-lauer added the bug Something isn't working label Feb 4, 2022
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

I like it!

In my opinion we could even go further and completely ignore scalar coordinates altogether (i.e., removing them before the merge and re-adding them later if necessary). I've had so many troubles with them (e.g., SciTools/iris#3584, just today!)...

However this might be a topic for another pull request. As far as I understood there might be also better support by iris in the future SciTools/iris#4446).

axel-lauer and others added 2 commits February 7, 2022 09:52
@schlunma schlunma added this to the v2.5.0 milestone Feb 7, 2022
@schlunma schlunma changed the title Fix multi-model statistics for variables on hybrid levels Removed scalar coordinates p0 and ptop prior to merge in multi_model_statistics Feb 7, 2022
@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #1471 (e10578b) into main (c788735) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1471   +/-   ##
=======================================
  Coverage   89.84%   89.84%           
=======================================
  Files         196      196           
  Lines       10473    10477    +4     
=======================================
+ Hits         9409     9413    +4     
  Misses       1064     1064           
Impacted Files Coverage Δ
esmvalcore/cmor/_fixes/shared.py 100.00% <ø> (ø)
esmvalcore/preprocessor/_multimodel.py 95.50% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c788735...e10578b. Read the comment docs.

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

🎉

(Failing tests are not related to this PR)

@schlunma schlunma mentioned this pull request Feb 7, 2022
9 tasks
@axel-lauer
Copy link
Contributor Author

Just tested this again and it works nicely. This can be merged now.

@schlunma
Copy link
Contributor

schlunma commented Feb 8, 2022

Great, thanks Axel! Just merged the main into this to make the tests pass, will merge afterwards.

@schlunma schlunma merged commit 7c33581 into main Feb 8, 2022
@schlunma schlunma deleted the fix_multimodel_hybrid branch February 8, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with new multi-model statistics and variables on hybrid levels
2 participants