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

Use std instead of std_dev in recipe_esacci_lst.yml #2522

Closed
wants to merge 2 commits into from

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Feb 7, 2022

Description

std_dev is not supported as multimodel statistic, replacing it with std


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.

@zklaus
Copy link

zklaus commented Feb 7, 2022

I think this is wrong. Please have a look at https://github.com/ESMValGroup/ESMValCore/blob/7c58c80dbb5c53147ccb4b20b71200ccfae4adf0/esmvalcore/preprocessor/_multimodel.py#L57. We need to understand better what is going on before we can jump into mucking with the code.

@schlunma
Copy link
Contributor

schlunma commented Feb 7, 2022

I think this is wrong. Please have a look at https://github.com/ESMValGroup/ESMValCore/blob/7c58c80dbb5c53147ccb4b20b71200ccfae4adf0/esmvalcore/preprocessor/_multimodel.py#L57. We need to understand better what is going on before we can jump into mucking with the code.

Agreed. Looks like ESMValGroup/ESMValCore#673 undid some of the changes from ESMValGroup/ESMValCore#1150. This needs be fixed in the core.

BTW, I thought the CI runs use the released version of the core? Why did the merge of ESMValGroup/ESMValCore#673 affect this?

@schlunma
Copy link
Contributor

schlunma commented Feb 7, 2022

I will open a PR for that in the core

@valeriupredoi
Copy link
Contributor Author

thanks guys, I be closing this then! Manu, there is one (powerful) GA test that runs the tests with the latest Core main and that's the only one affected via Core/673

@valeriupredoi valeriupredoi deleted the use_std branch February 7, 2022 15:37
@zklaus
Copy link

zklaus commented Feb 7, 2022

And good thing we have it. Thanks for that, @valeriupredoi ! You're a 🌟

PS: This is the kind of stuff that would be avoided or at least much less likely with a workflow that is based on rebase instead of merge.

@schlunma schlunma restored the use_std branch February 7, 2022 15:48
@valeriupredoi
Copy link
Contributor Author

And good thing we have it. Thanks for that, @valeriupredoi ! You're a star2

shucks 😊

Yeah, agreed about rebasing, only problem I see is users that are not that familiar with github getting stuck with it and being scared s**less of the errors they may get from doing that. Or we can just do it at merge point on GH only?

@zklaus
Copy link

zklaus commented Feb 7, 2022

I mean the point is to keep the history clean at all times and make really clear which changes are actually coming from a particular PR. Not much point in doing this after everything has been thoroughly mangled over years of merging. Also, very difficult to do at that point.

@schlunma schlunma deleted the use_std branch April 5, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using std_dev as multimodel statistic is no longer accepted after ESMValCore PR 673 got merged
3 participants