Skip to content

Implementation of ensemble member recognition to the ClimWIP diagnostic #1852

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

Merged
merged 12 commits into from
Nov 24, 2020

Conversation

lukasbrunner
Copy link
Contributor

@lukasbrunner lukasbrunner commented Oct 9, 2020

Before you start, please read our contribution guidelines.

Please discuss your idea with the development team before getting started, to avoid disappointment later. The way to do this is to open a new issue on GitHub. If you are planning to modify an existing functionality, please discuss it with the original author(s) by tagging them in the issue.


Tasks

  • Give this pull request a descriptive title that can be used as a one line summary in a changelog
  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • Make sure your code is composed of functions of no more than 50 lines and uses meaningful names for variables
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes

Modified recipe/diagnostic

  • Update documentation for the recipe to the doc/sphinx/source/recipes folder
  • Update provenance information if needed
  • Assign the author(s) of the affected recipe(s) as reviewer(s)

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes #1849

@Peter9192
Copy link
Contributor

@lukasbrunner is this ready for review? If so, can you mark it as such and request a review from me?

@lukasbrunner lukasbrunner marked this pull request as ready for review November 12, 2020 10:21
@lukasbrunner
Copy link
Contributor Author

okay I changed the processing order a bit. actually the ensemble means should probably only be done just before the weighting. this means that all the intermediate plots still show individual members. the order i had originally led to the mean plots and the plots for the individual variable groups being different.

this is probably also the cleanest form thinking forward to the implementation of the perfect model test

Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Hey @lukasbrunner ,
I had a look at the code. It works, and I think the main approach (combining before the weights, splitting after) is fine. But I had a bit of a hard time understanding all the logic to keep track of which ensemble members are part of which models, and I hope we can make that a bit simpler. I left some comments/suggestions to this effect. Do you think we can use some of that?

Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Looks great now! Just one more small suggestion.

I guess we need a scientific review and a test bot run. @nielsdrost can you call the bot on this recipe? Then @ruthlorenz can use the 'official' output in her scientific check.

lukasbrunner and others added 2 commits November 17, 2020 16:55
@nielsdrost
Copy link
Member

@esmvalbot recipe_climwip.yml

@esmvalbot
Copy link

esmvalbot bot commented Nov 18, 2020

Starting recipe recipe_climwip.yml, output will be generated here

@esmvalbot
Copy link

esmvalbot bot commented Nov 18, 2020

Recipe recipe_climwip.yml ran OK, output has been generated here

@lukasbrunner
Copy link
Contributor Author

@nielsdrost The bot ran through, can you please push the button :)

@ruthlorenz
Copy link
Contributor

Sorry, my review still hanging. Testing now.

Does this now depend on the recipe being set-up correctly as in reading all model names (=datasets) from there?
While this clearly makes things simpler codewise, there might be issues. E.g for CMIP6 data I just found that CESM2 for ssp585 has ensembles r1, r2, r4, r10 and r11. So I would fill in the recipe as:

    - {dataset: CESM2, project: CMIP6, exp: [historical, ssp585], grid: gn, ensemble: r(1:2)i1p1f1}
    - {dataset: CESM2, project: CMIP6, exp: [historical, ssp585], grid: gn, ensemble: r4i1p1f1}
    - {dataset: CESM2, project: CMIP6, exp: [historical, ssp585], grid: gn, ensemble: r(10:11)i1p1f1}

Would that still be handled correctly?
(actually this is running at the moment, but too slow and I need to run in 2 mins...so you can maybe check or answer already ;-))

@lukasbrunner
Copy link
Contributor Author

Do you mean the order of input? Lets wait for the result, but I think it should be fine. I remember raising this with Peter before and he now calls natsorted on them at some point so the recipe input order should not matter. If you want to be sure you could even put another model in between. (if that is what you were asking?)

@Peter9192
Copy link
Contributor

Would that still be handled correctly?

I think it should be fine, because what you're writing in the recipe is just shorthand yaml that will get expanded as soon as ESMValTool starts working with the files. When we read in the data in the diagnostic script, we reconstruct the model+ensemble+experiment name from the metadata provided. This happens here:

identifier_fmt='{dataset}_{ensemble}_{exp}')

Copy link
Contributor

@ruthlorenz ruthlorenz left a comment

Choose a reason for hiding this comment

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

Tested this with a bunch of CMIP6 Models with quite a few ensembles and it does the right thing.
weights.pdf

@lukasbrunner
Copy link
Contributor Author

It would be great if this could get merged for tomorrow. We aim to work on something that depends on it during the workshop :) The bot run though and we have two positive reviews. @bouweandela @nielsdrost Thanks!

@bouweandela bouweandela merged commit 03c9778 into master Nov 24, 2020
@bouweandela bouweandela deleted the climwip_ensemble_members branch November 24, 2020 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostic EUCP www.eucp-project.eu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ensemble member recognition to the ClimWIP diagnostic
5 participants