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

Smarter removal of coordinate metadata in multi_model_statistics preprocessor #1813

Merged
merged 13 commits into from
Feb 1, 2023

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Nov 21, 2022

Description

This PR makes the removal of coordinate metadata in multi_model_statistics preprocessor smarter, i.e., it uses common names (var_name, standard_name, long_name) for coordinates whose name() and units match.

Prior to that change, the code removed the long_name attribute for all coordinates that did not exactly match, which caused problems for coordinates that only had a long name (e.g., the ones coming out of iris.coord_categorisation).

Note that this does not apply if two or more coordinates of a cube have the same name().

In addition, now also the circular attribute is removed for all dimensional coordinates (this may be set by some models and by some not, which prevented multi_model_statistics to run).

Case Old logic New logic
Exactly identical coordinates Remain unchanged Remain unchanged
Coordinates with matching name() and units Delete long_name and attributes Set common var_name, standard_name, long_name, delete attributes, and set circular=False for dimensional coordinates
All remaining coordinates Delete long_name and attributes Delete long_name and attributes, and set circular=False for dimensional coordinates

Closes #1807

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:

@schlunma schlunma added the enhancement New feature or request label Nov 21, 2022
@schlunma schlunma added this to the v2.8.0 milestone Nov 21, 2022
@schlunma schlunma self-assigned this Nov 21, 2022
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Merging #1813 (c54dc58) into main (d832c8c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1813      +/-   ##
==========================================
+ Coverage   92.03%   92.05%   +0.02%     
==========================================
  Files         234      234              
  Lines       12042    12074      +32     
==========================================
+ Hits        11083    11115      +32     
  Misses        959      959              
Impacted Files Coverage Δ
esmvalcore/preprocessor/_multimodel.py 97.70% <100.00%> (+0.32%) ⬆️

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

@schlunma schlunma marked this pull request as ready for review November 30, 2022 13:03
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.

nice indeed, bar one (mostly philosophical) question: are we looking for full uniformity in metadata, or we can do something smart like: if 90% of datasets have a certain value for an attribute, then we'll use that (see inline comment specific to this question)

esmvalcore/preprocessor/_multimodel.py Show resolved Hide resolved
@valeriupredoi
Copy link
Contributor

@ESMValGroup/technical-lead-development-team can one of you lovlies have a last look and merge this please? 🍺

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.

Removal of coordinate metadata in multi_model_statistics is too strict
3 participants