Allow multi_model_statistics on cubes with arbitrary dimensions
#1808
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1808 +/- ##
==========================================
+ Coverage 91.50% 91.53% +0.03%
==========================================
Files 202 202
Lines 10919 10938 +19
==========================================
+ Hits 9991 10012 +21
+ Misses 928 926 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Co-authored-by: Bettina Gier <[email protected]>
|
Successfully runs in my WIP diagnostic where I used a selfmade mmm workaround before, so thumbs up from me! |
There was a problem hiding this comment.
very very nice PR and with loads of functionality, great work @schlunma - I would nominate this for the v2.8 Highlights BTW. Just a few questions from me, nothing that warrants requesting changes, but it'd be good to have answers. Even though the testing is bang on, I'd still want to have a recipe or two run - say, two recipes that use MM and have been plagued with issues you fixed here perhaps?
|
Thanks for the review @valeriupredoi !! I tested 2 custom recipes that benefit from the new features, and @bettina-gier also tested one. All of them work as expected! Regarding recipes in our repo - since they work with previous versions of the tool they do not suffer from the issues solved here. However, I think many recipes/diagnostics could now be simplified with this change, but that's a different story. To be safe, I will run 2-3 recipes with the old/new code and compare them 👍 |
|
That's great, cheers, Manu!
Awesome! I should have run those as a reviewer, but after the release I have now developed a chronic aversion of running anything 🤣 And since you're on the job... |
|
@ESMValGroup/technical-lead-development-team once Manu has finisjed running the extra recipes for testing, could one of yous please have a final looksee and merge this? It's a really neat PR! |
valeriupredoi
added
preprocessor
|
|
|
@schlunma just noticed #1811 (comment) - is that still a blocker for this PR? |
|
No, just for the local recipe I used for testing. Merged the two branches together locally, all fine 👍 (But if you have 5 min to have a quick look at #1811 it would also be much appreciated 👍) |
|
perfect, thanks, Manu! I will, as soon as I fix the PR on the feedstock, in 10min 👍 |
giddyup folks! 🍺 |
|
Great to see these problems finally fixed @schlunma, awesome job! |
|
@bouweandela I think this can be merged now 👍 |
|
I agree, but I didn't merge right away because these are fairly substantial changes, so I wanted to give other members of the @ESMValGroup/technical-lead-development-team the opportunity to comment. |
Description
This PR allows the usage of the
multi_model_statisticspreprocessor on data with arbitrary dimensions, i.e., it's not necessary anymore that data has time and/or horizontal dimensions.In addition, it converts the
pytest.mark.xfailused in the tests topytest.raisessince the corresponding tests are not expected to fail due to a bug. We rather expect the corresponding errors to appear due to different coordinates/shapes in the input cubes.Pytest doc:
Closes #1009
Closes #890
Closes #891
Closes #738
Closes #41
Link to documentation: https://esmvaltool--1808.org.readthedocs.build/projects/ESMValCore/en/1808/recipe/preprocessor.html?highlight=multi_model_stat#multi-model-statistics
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: