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

Allowed ignoring scalar coordinates in multi_model_statistics #1934

Merged
merged 4 commits into from
Feb 17, 2023

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Feb 16, 2023

Description

This PR allows the optional keyword ignore_scalar_coords for the multi_model_statistics and ensemble_statistics preprocessors. With this setting enabled, different scalar coords in the input cubes are ignored and do not raise merge MergeErrors.

Closes #1606

Link to documentation: https://esmvaltool--1934.org.readthedocs.build/projects/ESMValCore/en/1934/recipe/preprocessor.html#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:

@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #1934 (6c587dd) into main (c25b930) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1934   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files         234      234           
  Lines       12205    12213    +8     
=======================================
+ Hits        11251    11259    +8     
  Misses        954      954           
Impacted Files Coverage Δ
esmvalcore/_recipe/check.py 91.26% <100.00%> (+0.27%) ⬆️
esmvalcore/preprocessor/_multimodel.py 97.88% <100.00%> (+<0.01%) ⬆️

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

@schlunma schlunma self-assigned this Feb 16, 2023
@schlunma schlunma added the enhancement New feature or request label Feb 16, 2023
@schlunma schlunma assigned schlunma and unassigned schlunma Feb 16, 2023
@schlunma schlunma added this to the v2.8.0 milestone Feb 16, 2023
Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

very cool, Manu! Just a couple of very minor review points from me 🍺 Would we want to extend that [p0, ptop] list of coords to be always removed? If so, I'd put that at the top of the mm script, for now, maybe we'll take it out from there and expose it to the API some time in the future. About boolean (true or false) - it's always nice to tell the users stuff in layman terms, you know 😁

esmvalcore/_recipe/check.py Outdated Show resolved Hide resolved
esmvalcore/preprocessor/_multimodel.py Show resolved Hide resolved
tests/integration/recipe/test_check.py Outdated Show resolved Hide resolved
@schlunma
Copy link
Contributor Author

Thanks for the quick review @valeriupredoi!

Would we want to extend that [p0, ptop] list of coords to be always removed? If so, I'd put that at the top of the mm script, for now, maybe we'll take it out from there and expose it to the API some time in the future.

I don't think we should expose this to the public API, especially with that new option now. We can also extend that list in there 👍

@valeriupredoi
Copy link
Contributor

Thanks for the quick review @valeriupredoi!

Would we want to extend that [p0, ptop] list of coords to be always removed? If so, I'd put that at the top of the mm script, for now, maybe we'll take it out from there and expose it to the API some time in the future.

I don't think we should expose this to the public API, especially with that new option now. We can also extend that list in there +1

Perfect then! Even less knobs and buttons for users to deal with it - I like it. Great work on this, hope signor Lovato is 'appy now, after we've left him out for dry for such a long time 😁

@tomaslovato
Copy link
Contributor

thanks a lot for the quick response @schlunma & @valeriupredoi

@valeriupredoi
Copy link
Contributor

@ESMValGroup/technical-lead-development-team this is niice (in Borat accent) and ready for merging - please have a look and merge at earliest convenience, if you dillydally about it, myself or @remi-kazeroni will merge (given tight schedule on 2.8 freeze)

@zklaus
Copy link

zklaus commented Feb 17, 2023

It's a little bit muddled, isn't it? The documentation states

To ignore different scalar coordinates in the input datasets, [...]

and

Differing scalar coordinates can be ignored with the option ignore_scalar_coords.

but it's not quite clear what "ignore" really means and the code just removes all scalar coordinates.
This is stated correctly down in the notes on the API docs, where it says

If True, remove any scalar coordinate in the input datasets before merging the input cubes into the multi-dataset cube. [...]

In lieu of implementing the behavior suggested by the documentation, i.e. removing only the differing scalar coordinates, I suggest clarifying the documentation in the two places mentioned above to make it easier to understand for the user what is happening.

@valeriupredoi
Copy link
Contributor

valeriupredoi commented Feb 17, 2023

bit of a 🐔 - 🥚 problem, @zklaus -> ignoring stuff in input cubes means said stuff is not present in output cube(s) - that sounds very logical to me as @schlunma wrote it, but I would indeed change:

To ignore different scalar coordinates in the input datasets, use the option
ignore_scalar_coords: true.

with

To ignore all scalar coordinates in the input datasets, use the option
ignore_scalar_coords: true; this way these scalar coordinates will not be saved to the output multimodel cube.

@schlunma
Copy link
Contributor Author

@zklaus I tried to improve the documentation, could you take another look? Thanks!

@zklaus zklaus merged commit 76737d5 into main Feb 17, 2023
@zklaus zklaus deleted the mm_stats_ignore_scalar_coords branch February 17, 2023 15:51
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.

Preprocessor multimodel fails due to different scalar aux_coords
4 participants