Skip to content

Add option to exclude input cubes in output of multimodel statistics#978

Merged
valeriupredoi merged 4 commits intomasterfrom
fix_975
Feb 2, 2021
Merged

Add option to exclude input cubes in output of multimodel statistics#978
valeriupredoi merged 4 commits intomasterfrom
fix_975

Conversation

@Peter9192
Copy link
Contributor

@Peter9192 Peter9192 commented Feb 2, 2021

Description


Before you get started

Checklist


To help with the number pull requests:

@bouweandela bouweandela changed the title Add option to include input cubes in output of multimodel statistics Add option to exclude input cubes in output of multimodel statistics Feb 2, 2021
@bouweandela
Copy link
Member

It would also be good to add a unit test to ensure that this doesn't get broken again in the future.

@bouweandela bouweandela added this to the v2.2.0 milestone Feb 2, 2021
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
statistics=statistics,
output_products=output_products,
span=span,
keep_originals=keep_originals,
Copy link
Contributor

@stefsmeets stefsmeets Feb 2, 2021

Choose a reason for hiding this comment

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

If this is never going to be exposed and only applies to PreprocessorFile, why not fix it in the public function?

Suggested change
keep_originals=keep_originals,
statistics_products = _multiproduct_statistics(
products=products,
statistics=statistics,
output_products=output_products,
span=span,
)
if keep_originals:
return products | statistics_products
else:
return statistics_products

Copy link
Contributor

Choose a reason for hiding this comment

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

@stefsmeets when you suggest code changes via the Github interactive editor there is no need to add text formatting syntax (like ```python here). Also @Peter9192 I would change keep_originals to something like save_input_cubes since originals refers more to the initial un-preprocced data and the option name doesn't tell the user what it does. Also note that for this to be in the documentation needs a mention of it 👍

@valeriupredoi
Copy link
Contributor

OK I confirm that this fixes #975 - before and after fix:

(iris3) [valeriu@sci2 esmvaltool_var_test]$ ls -la eo1/recipe_python_20210202_112301/preproc/diagnostic1/ta/
total 18284
drwxr-xr-x 2 valeriu users       0 Feb  2 11:23 .
drwxr-xr-x 4 valeriu users       0 Feb  2 11:23 ..
-rw-r--r-- 1 valeriu users    1352 Feb  2 11:23 metadata.yml
-rw-r--r-- 1 valeriu users 9360338 Feb  2 11:23 MultiModelMean_Amon_ta_2000-2002.nc
-rw-r--r-- 1 valeriu users 9360338 Feb  2 11:23 MultiModelMedian_Amon_ta_2000-2002.nc
(iris3) [valeriu@sci2 esmvaltool_var_test]$ ls -la eo2/recipe_python_20210202_113151/preproc/diagnostic1/ta/
total 36584
drwxr-xr-x 2 valeriu users       0 Feb  2 11:32 .
drwxr-xr-x 4 valeriu users       0 Feb  2 11:32 ..
-rw-r--r-- 1 valeriu users 9370329 Feb  2 11:32 CMIP5_CanESM2_Amon_historical_r1i1p1_ta_2000-2002.nc
-rw-r--r-- 1 valeriu users 9367345 Feb  2 11:32 CMIP5_MPI-ESM-LR_Amon_historical_r1i1p1_ta_2000-2002.nc
-rw-r--r-- 1 valeriu users    2846 Feb  2 11:32 metadata.yml
-rw-r--r-- 1 valeriu users 9360338 Feb  2 11:32 MultiModelMean_Amon_ta_2000-2002.nc
-rw-r--r-- 1 valeriu users 9360338 Feb  2 11:32 MultiModelMedian_Amon_ta_2000-2002.nc

@Peter9192 cheers for the very quick fix, we need to get this in v2.2 pronto @jvegasbsc - we need a test this functionality is preserved as @stefsmeets says, please add it here 🍺

@bouweandela bouweandela added the bug Something isn't working label Feb 2, 2021
@Peter9192
Copy link
Contributor Author

Here's a very simple test. I don't have any more time right now.

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.

looks good to me, cheers @Peter9192 🍺

Copy link
Member

@LisaBock LisaBock left a comment

Choose a reason for hiding this comment

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

Works for me as well! Thanks!

@valeriupredoi valeriupredoi merged commit fd70226 into master Feb 2, 2021
@valeriupredoi valeriupredoi deleted the fix_975 branch February 2, 2021 13:01
bettina-gier pushed a commit that referenced this pull request Feb 16, 2021
…978)

* Add option (default True) to keep original cubes

* Better docstring

Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>

* Superficial test to verify the output products of multimodel statistics

* slight reformulation of conditional

Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi model statistics: missing files for individual models

5 participants