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

Add "grid" as a tag to the output file template for CMIP6 #1356

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

zklaus
Copy link

@zklaus zklaus commented Oct 11, 2021

Description

Closes #1134

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:

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #1356 (21bc3c5) into main (40e1a35) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1356   +/-   ##
=======================================
  Coverage   88.26%   88.26%           
=======================================
  Files         194      194           
  Lines        9836     9836           
=======================================
  Hits         8682     8682           
  Misses       1154     1154           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40e1a35...21bc3c5. Read the comment docs.

@zklaus zklaus marked this pull request as ready for review October 12, 2021 07:51
@@ -28,7 +28,7 @@ CMIP6:
ETHZ: '{exp}/{mip}/{short_name}/{dataset}/{ensemble}/{grid}/'
SYNDA: '{activity}/{institute}/{dataset}/{exp}/{ensemble}/{mip}/{short_name}/{grid}/{latestversion}'
input_file: '{short_name}_{mip}_{dataset}_{exp}_{ensemble}_{grid}*.nc'
output_file: '{project}_{dataset}_{mip}_{exp}_{ensemble}_{short_name}'
output_file: '{project}_{dataset}_{mip}_{exp}_{ensemble}_{short_name}_{grid}'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs a bit more attention: is grid now a mandatory parameter? If it is, we need to add an exception in _recipe.py in the form of a RecipeError if it's not provided

Copy link
Author

Choose a reason for hiding this comment

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

I think for CMIP6 grid has always been mandatory. Do you have a recipe that runs without a specified grid?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's mandatory while data finder runs, but this can be promoted to before that via a RecipeError rather than having the run crash with Missing data when no grid is specified:

2021-10-12 12:12:21,985 UTC [10195] ERROR   Could not create all tasks
2021-10-12 12:12:21,985 UTC [10195] ERROR   Missing data for preprocessor validation_with_ERA-Interim/tas:
- Dataset key 'grid' must be specified for {'preprocessor': 'pp_rad', 'variable_group': 'tas', 'short_name': 'tas', 'diagnostic': 'validation_with_ERA-Interim', 'dataset': 'IPSL-CM6A-LR', 'project': 'CMIP6', 'mip': 'Amon', 'exp': 'historical', 'ensemble': 'r1i1p1f1', 'start_year': 1900, 'end_year': 2000, 'recipe_dataset_index': 0, 'institute': ['IPSL'], 'activity': 'CMIP', 'alias': 'CMIP6_IPSL-CM6A-LR', 'original_short_name': 'tas', 'standard_name': 'air_temperature', 'long_name': 'Near-Surface Air Temperature', 'units': 'K', 'modeling_realm': ['atmos'], 'frequency': 'mon', 'filename': '/home/users/valeriu/esmvaltool_var_test/esmvaltool_output/recipe_validation_CMIP6_20211012_121208/preproc/validation_with_ERA-Interim/tas/CMIP6_IPSL-CM6A-LR_Amon_historical_r1i1p1f1_tas_1900-2000.nc'}, check your recipe entry

Copy link
Contributor

@valeriupredoi valeriupredoi Oct 12, 2021

Choose a reason for hiding this comment

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

we can do that later, in a new PR if you think the current behavior is enough for now though 👍

Copy link
Member

Choose a reason for hiding this comment

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

Missing data is a RecipeError

Copy link
Member

Choose a reason for hiding this comment

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

I changed that recently, look at the code above. You may need to do a git pull if you still get a KeyError.

Copy link
Member

Choose a reason for hiding this comment

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

You're right we could catch this error before even looking for data, could you open a new issue suggesting that as an improvement?

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed that recently, look at the code above. You may need to do a git pull if you still get a KeyError.

ah awesome, cheers!

You're right we could catch this error before

Shall dos! But isn't that a trivial fix by adding grid to the required_keys list in _recipe.py (if we want to promote it to being so important, that is)

Copy link
Author

Choose a reason for hiding this comment

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

Feel free to put that information in the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but we could also be a bit smarter and extract required_keys from the project DRS

@bouweandela bouweandela merged commit 3c3f5cb into main Oct 15, 2021
@bouweandela bouweandela deleted the add-grid-to-output_file-template branch October 15, 2021 08:52
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.

CMIP6 datasets that only differ in grid type cannot be used in a single recipe variable section
3 participants