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

Made equalized attributes in concatenated cubes consistent across runs #1783

Merged
merged 13 commits into from
Feb 9, 2023

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Nov 4, 2022

Description

This PR fixes the ordering attributes for concatenated cubes from multiple source (e.g., multiple experiments). If values are equal across cubes, this value is not changed. If values differ, they are converted to strings, sorted, and joined by a delimiter (default: |). Example: exp: historical|ssp585.

Closes #1779

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 bug Something isn't working label Nov 4, 2022
@schlunma schlunma added this to the v2.8.0 milestone Nov 4, 2022
@schlunma schlunma self-assigned this Nov 4, 2022
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #1783 (559af1b) into main (684e7ea) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1783   +/-   ##
=======================================
  Coverage   92.10%   92.11%           
=======================================
  Files         234      234           
  Lines       12133    12144   +11     
=======================================
+ Hits        11175    11186   +11     
  Misses        958      958           
Impacted Files Coverage Δ
esmvalcore/iris_helpers.py 100.00% <100.00%> (ø)
esmvalcore/preprocessor/_io.py 84.10% <100.00%> (-0.64%) ⬇️
esmvalcore/typing.py 100.00% <100.00%> (ø)

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 4, 2022 11:18
@zklaus
Copy link

zklaus commented Nov 4, 2022

I like this functionality, but I have three main comments. First, the typical delimiter used in CF is simply " ", i.e. a space, which is not great for many reasons, but still worth considering. It may create problems for some variable values, but for the typical experiment id and variant label, etc., this would be fine and consistent with CF. Second, the name equalize clearly alludes to Iris' own iris.util.equalise_attributes. Differences in spelling aside (fine, IMHO), this does follow different semantics. A different name might be better (join_attributes? merge_attributes?). Third, due to the proximity to the Iris functionality and the ongoing work on making concatenation and merging more forgiving in Iris, I think we should at least ping the Iris team and see if later on they might be interested in upstreaming this, either in the concatenation itself or perhaps via the iris.common.resolve functionality.

@schlunma
Copy link
Contributor Author

schlunma commented Nov 4, 2022

Thanks for your input @zklaus!

First, the typical delimiter used in CF is simply " ", i.e. a space, which is not great for many reasons, but still worth considering. It may create problems for some variable values, but for the typical experiment id and variant label, etc., this would be fine and consistent with CF.

Fine for me, I don't really have a strong opinion on this. I will change that.

Second, the name equalize clearly alludes to Iris' own iris.util.equalise_attributes. Differences in spelling aside (fine, IMHO), this does follow different semantics. A different name might be better (join_attributes? merge_attributes?).

Yes, that definitely makes sense. I will adapt the name.

Third, due to the proximity to the Iris functionality and the ongoing work on making concatenation and merging more forgiving in Iris, I think we should at least ping the Iris team and see if later on they might be interested in upstreaming this, either in the concatenation itself or perhaps via the iris.common.resolve functionality.

Good idea! I will open an issue and ask if this feature is something that might be interesting for them. Let's wait for their response until we merge this to avoid having to deprecate this in the next release 😄

@schlunma
Copy link
Contributor Author

schlunma commented Nov 4, 2022

See SciTools/iris#5050

@schlunma schlunma requested a review from zklaus November 4, 2022 14:40
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Nice work @schlunma! I left some comments on how the code could be further improved, but it looks fine as it is already.

esmvalcore/iris_helpers.py Outdated Show resolved Hide resolved
esmvalcore/iris_helpers.py Outdated Show resolved Hide resolved
esmvalcore/iris_helpers.py Outdated Show resolved Hide resolved
@schlunma schlunma marked this pull request as draft November 22, 2022 10:12
@schlunma schlunma modified the milestones: v2.8.0, v2.9.0 Jan 3, 2023
@schlunma
Copy link
Contributor Author

I wonder if we also should include this in v2.8. Looks like there won't be a solution in iris anytime soon, and this will be a further steps towards reproducible results.

@schlunma schlunma modified the milestones: v2.9.0, v2.8.0 Jan 27, 2023
@schlunma schlunma marked this pull request as ready for review January 27, 2023 13:06
esmvalcore/typing.py Outdated Show resolved Hide resolved
esmvalcore/typing.py Outdated Show resolved Hide resolved
@valeriupredoi
Copy link
Contributor

Good idea! I will open an issue and ask if this feature is something that might be interesting for them. Let's wait for their response until we merge this to avoid having to deprecate this in the next release

You still want to do this @schlunma ? Otherwise I can merge this unless you are still waiting for an OK from @zklaus 🍺

@schlunma
Copy link
Contributor Author

schlunma commented Feb 9, 2023

I opened the issue here SciTools/iris#5050, but I think we can merge it, does not look like we get an iris solution anytime soon

@valeriupredoi valeriupredoi added the iris Related to the Iris package label Feb 9, 2023
@valeriupredoi valeriupredoi merged commit c27a9b5 into main Feb 9, 2023
@valeriupredoi valeriupredoi deleted the consistent_fix_attributes branch February 9, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working iris Related to the Iris package
Projects
None yet
4 participants