Skip to content

Add ensemble statistics preprocessor and 'groupby' option for multimodel #673

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.

Sign up for GitHub

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

Jump to bottom
Merged
schlunma merged 182 commits into from
Feb 7, 2022
Merged

Add ensemble statistics preprocessor and 'groupby' option for multimodel #673

schlunma merged 182 commits into from
Feb 7, 2022

Conversation

@sloosvel
Copy link
Contributor

@sloosvel sloosvel commented Jun 10, 2020 •
edited by schlunma
Loading

  • Add a new preprocessor function to calculate ensemble statistics
  • Add support for grouping in multi-model statistics preprocessor
  • Separate functions that operate on cubes from functions that operate on products (esmvalcore.PreprocessorFiles).
  • More efficient implementation of multi-cube statistics that can eventually be used for multi-model statistics as well

Link to documentation: https://esmvaltool--673.org.readthedocs.build/projects/ESMValCore/en/673/recipe/preprocessor.html#ensemble-statistics

Tasks

  • 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)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. 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
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

Closes #52
Merge first: #685

@sloosvel sloosvel added the preprocessor Related to the preprocessor label Jun 10, 2020
@sloosvel sloosvel requested a review from Peter9192 June 10, 2020 13:30
Peter9192 added 21 commits June 23, 2020 11:58
@Peter9192
Fix calendar units to 'days since 1850-01-01' on a standard calendar
8e914e7
@Peter9192
More thorough check on source time frequency; align behaviour with re…
d6b9947 
…grid time; raise for daily data
@Peter9192
Simplify _get_overlap
12f9aa4
@Peter9192
Align behaviour for union (full) and intersection (overlap) of time a…
bf82085 
…rrays
@Peter9192
Add function to make all cubes use the same calendar.
3bd1e82 
This function also checks the frequency of the cubes (previously in _datetime_to_int_days),
this seems to be a much more logical place.

_datetime_to_int_days now simplifies to a one-liner, and can probably be eliminated completely.
@Peter9192
Remove _datetime_to_int_days, as we can just use the time points
7841a5f
@Peter9192
Simplify and rename _slice_cube
f95aa7c
@Peter9192
Align _assemble_overlap_data more with _assemble_full_data
db47522
@Peter9192
Align _assemble_full_data more with _assemble_overlap_data
127c69b
@Peter9192
Futher align assemble full and overlap data
f873ea5
@Peter9192
Merge assemble full and overlap data.
3b7b55b
@Peter9192
Further simplify
168a701
@Peter9192
Remove stuff about bounds and aux coords as it is not used anyway
629a9d6
@Peter9192
Remove stuff about bounds and aux coords as it is not used anyway
f538fa8
@Peter9192
Clean up tests and add tests for new functions
b0cc1ae
@Peter9192
Valeriu's suggestions
aef2578
@Peter9192
fix tests
24294df
@Peter9192
Merge remote-tracking branch 'origin/master' into mmstats_simplify_time
d3a4ddd
@Peter9192
Realize data before making time slices
35210a5
@Peter9192
Avoid codacy 'pointless-statement' message
d2fddeb
@Peter9192
Merge remote-tracking branch 'origin/master' into mmstats_simplify_time
d028b28
@schlunma
Copy link
Contributor

schlunma commented Feb 4, 2022

@Peter9192 Agreed, makes sense. In this case I would suggest to add a note to the documentation about this:

  1. For multi_model_statistics, add 1 sentence about keep_input_datasets (this is not described yet)
  2. Add the following nice example from @Peter9192 to ensemble_statistics with something along the lines "If a diagnostic requires ensemble means (or other statistics) AND the individual ensemble members, you can use the following preprocessor combination in the recipe:"
preprocessors:
  everything_else: &everything_else
    area_statistics: ... 
    regrid_time: ...  
  multimodel:
    <<: *everything_else
    ensemble_statistics:
      ...

variables:
  tas_all_cubes:
    short_name: tas
    preprocessor: everything_else
    ...
  tas_mmstats:
    short_name: tas
    preprocessor: multimodel
    ...

Maybe also add a short .. note:: that explicitly states that ensemble_statistics does not support keep_input_datasets?

@schlunma schlunma mentioned this pull request Feb 4, 2022
Closed
schlunma
schlunma reviewed Feb 4, 2022
View reviewed changes
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Great! I tested this preprocessor extensively with this recipe and everything works perfectly! Many thanks!!

I just have some additional comments regarding the doc, afterwards this can be merged 🎉

@Peter9192
Copy link
Contributor

Peter9192 commented Feb 4, 2022

Just wanted to step in and say awesome work here @sloosvel! Thanks for picking up this PR. Also cheers to @schlunma and @valeriupredoi for kicking and reviewing it. Have a nice weekend!

@valeriupredoi
Copy link
Contributor

valeriupredoi commented Feb 4, 2022

yeah man, I second that! Good weekend too, 30min til pub for me 😁 🍺

@schlunma
Copy link
Contributor

schlunma commented Feb 7, 2022 •
edited
Loading

The documentation build fails with a

failed to reach any of the inventories with the following issues:
intersphinx inventory 'https://docs.scipy.org/doc/scipy/reference/objects.inv' not fetchable due to <class 'requests.exceptions.HTTPError'>: 404 Client Error: Not Found for url: https://docs.scipy.org/doc/scipy/reference/objects.inv

now. This is most certainly not related to this PR.

Any tips on this @ESMValGroup/esmvaltool-coreteam? I don't think scipy migrated their doc, so I guess this is just a temporary problem. Will try again in a couple of hours, but I think we should merge this even when this test is failing. The commit before that passed all tests.

EDIT: Just realized that the nightly build failed, too, so this is definitely not related to this PR.

@zklaus
Copy link

zklaus commented Feb 7, 2022

I came to the same conclusion as @schlunma regarding the broken intersphinx link. It is possible that we need to change something (see scipy/scipy#14267), but let's wait for a bit until things get clearer.

schlunma
schlunma approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Merging this now since the failing tests are not related to this PR.

Thanks for all your work on this @sloosvel and @Peter9192, this is a great addition to the tool! 🎉

@schlunma schlunma merged commit 3ee96f4 into main Feb 7, 2022
@schlunma schlunma deleted the dev_multimodel_ensemble branch February 7, 2022 10:47
@schlunma schlunma mentioned this pull request Feb 7, 2022
Merged
11 tasks
@zklaus zklaus mentioned this pull request Feb 7, 2022
Closed
@valeriupredoi valeriupredoi mentioned this pull request Feb 7, 2022
Closed
This was referenced Feb 7, 2022
Closed
Merged
@Peter9192
Copy link
Contributor

Peter9192 commented Feb 8, 2022

Awesome! Also thanks to @stefsmeets for your contributions 👍

@schlunma schlunma mentioned this pull request Feb 8, 2022
Closed
@sloosvel sloosvel mentioned this pull request Feb 8, 2022
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Peter9192 Peter9192 Peter9192 left review comments

@valeriupredoi valeriupredoi valeriupredoi approved these changes

@schlunma schlunma schlunma approved these changes

Assignees

No one assigned

Labels

preprocessor Related to the preprocessor

Projects

None yet

Milestone

v2.5.0 [closed]

Development

Successfully merging this pull request may close these issues.

Comparing several single model ensembles

8 participants

@sloosvel @stefsmeets @Peter9192 @bouweandela @schlunma @zklaus @valeriupredoi