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

Replace recipe_era5.yml with recipe_daily_era5.yml #2182

Merged
merged 23 commits into from
Aug 20, 2021

Conversation

SarahAlidoost
Copy link
Contributor

@SarahAlidoost SarahAlidoost commented May 27, 2021

Description


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.

New or updated recipe/diagnostic

New or updated data reformatting script


To help with the number of pull requests:

@SarahAlidoost SarahAlidoost changed the title Refactor era5 cmorizer Replace era5 cmorizer with recipe_daily_era5.yml Jun 24, 2021
@SarahAlidoost SarahAlidoost changed the title Replace era5 cmorizer with recipe_daily_era5.yml Replace recipe_era5.yml with recipe_daily_era5.yml Jun 24, 2021
@SarahAlidoost SarahAlidoost marked this pull request as ready for review June 28, 2021 15:10
SarahAlidoost and others added 2 commits July 23, 2021 15:33
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
doc/sphinx/source/recipes/recipe_cmorizers.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_cmorizers.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_cmorizers.rst Outdated Show resolved Hide resolved
doc/sphinx/source/input.rst Outdated Show resolved Hide resolved
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks, this is starting to look really good. I have a few more comments on the content of the documentation, but I think the structure is much improved now, especially if we get more supported datasets in the future.

doc/sphinx/source/recipes/recipe_cmorizers.rst Outdated Show resolved Hide resolved
doc/sphinx/source/input.rst Outdated Show resolved Hide resolved
SarahAlidoost and others added 7 commits July 26, 2021 11:42
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Fix capitalization of the word CMOR

Start new sentences on a new line

Igore vas with codespell
@bouweandela
Copy link
Member

@esmvalbot Please run cmorizers/recipe_daily_era5.yml

@esmvalbot
Copy link

esmvalbot bot commented Jul 26, 2021

Since @bouweandela asked, ESMValBot will run recipe cmorizers/recipe_daily_era5.yml as soon as possible, output will be generated here

@esmvalbot
Copy link

esmvalbot bot commented Jul 26, 2021

ESMValBot is sorry to report it failed to run recipe cmorizers/recipe_daily_era5.yml: exit is 1, output has been generated here

@bouweandela
Copy link
Member

It looks like the bot ran out of memory trying to run the recipe.
The broken tests on CircleCI are not related to this pull request.

@bouweandela
Copy link
Member

The recipe also runs out of memory on a 64 GB RAM machine, even with max_parallel_tasks: 1 in config-user.yml.

@SarahAlidoost
Copy link
Contributor Author

SarahAlidoost commented Aug 2, 2021

The recipe also runs out of memory on a 64 GB RAM machine, even with max_parallel_tasks: 1 in config-user.yml.

The recipe (including all variables) run with max_parallel_tasks: 1 was successful on Cartesius. here is more info from the run directory:
main_log_debug.txt
resource_usage.txt
main_log.txt

and more info from run/daily/rename directory:
log.txt
resource_usage.txt

and the environment file:
environment.txt

@remi-kazeroni
Copy link
Contributor

If this the right place for that, would it be possible to make the list of ERA5 variables for which cmorization is supported consistent between the recipes (recipe_check_obs.yml, recipe_daily_era5.yml) and the documentation?

Also this PR will add the MSWEP dataset to the list of supported datasets. For datasets for which we provide a specific cmorizer script, download instructions can usually be found in the script header. This is different for MSWEP for which I couldn't reproduce the steps to download. Could we work on that in this PR or should I open a specific issue for that? Thanks!

@bouweandela
Copy link
Member

Also this PR will add the MSWEP dataset to the list of supported datasets. For datasets for which we provide a specific cmorizer script, download instructions can usually be found in the script header. This is different for MSWEP for which I couldn't reproduce the steps to download. Could we work on that in this PR or should I open a specific issue for that? Thanks!

It would be great if you could open a specific issue for MSWEP, because this pull request is really about ERA5. We tried to reorganize the documentation a bit so things are more logically organized, but let's not let this pull request grow too large.

@bouweandela
Copy link
Member

The memory problems occurred for me with dask version 2021.7.1 (more than 64GB required). With version 2.30.0 of dask I was able to run the recipe successfully on Mistral (max memory usage 13GB). This might be something to look out for with other recipes too, we may need to pin the dask version..

If this the right place for that, would it be possible to make the list of ERA5 variables for which cmorization is supported consistent between the recipes (recipe_check_obs.yml, recipe_daily_era5.yml) and the documentation?

@SarahAlidoost Could you please have a look at this? Once the lists are consistent, this can be merged.

@SarahAlidoost
Copy link
Contributor Author

If this the right place for that, would it be possible to make the list of ERA5 variables for which cmorization is supported consistent between the recipes (recipe_check_obs.yml, recipe_daily_era5.yml) and the documentation?

thanks for your comment. The hourly variables in recipe_daily_era5.yml match all the hourly variables in the documentation, except ptype, rlns, rlus, and rsns, rsus. I think this is OK because this recipe only includes the variables for which we know how to convert hourly data to daily data.

Also, now the monthly and hourly variables in recipe_check_obs.yml match the variables in the documentation, except the hourly/monthly variables rlns, rsns and the hourly ones rlus, rsus. These were added in this PR. There is a cutom table for rlns and rsns.
To add these variables to recipe_check_obs.yml, we need the mip info:

var mip
hourly/monthly rlns ?
hourly/monthly rsns ?
hourly rlus ?
hourly rsus ?

@SarahAlidoost
Copy link
Contributor Author

The memory problems occurred for me with dask version 2021.7.1 (more than 64GB required). With version 2.30.0 of dask I was able to run the recipe successfully on Mistral (max memory usage 13GB). This might be something to look out for with other recipes too, we may need to pin the dask version..

If this the right place for that, would it be possible to make the list of ERA5 variables for which cmorization is supported consistent between the recipes (recipe_check_obs.yml, recipe_daily_era5.yml) and the documentation?

@SarahAlidoost Could you please have a look at this? Once the lists are consistent, this can be merged.

Please see my comment.

@bouweandela
Copy link
Member

To add these variables to recipe_check_obs.yml, we need the mip info:

@SarahAlidoost I think you can just use the usual Amon for monthly and E1hr for the hourly variables. For the latter it might be good to set the frequency too in the recipe, since these are radiation variables they are probably accumulated and the frequency should be 1hr. @lukasbrunner Since you added these variables in ESMValGroup/ESMValCore#1052, please correct me if I'm wrong.

@bouweandela
Copy link
Member

If this the right place for that, would it be possible to make the list of ERA5 variables for which cmorization is supported consistent between the recipes (recipe_check_obs.yml, recipe_daily_era5.yml) and the documentation?

@remi-kazeroni Has your comment been addressed now?

@remi-kazeroni
Copy link
Contributor

If this the right place for that, would it be possible to make the list of ERA5 variables for which cmorization is supported consistent between the recipes (recipe_check_obs.yml, recipe_daily_era5.yml) and the documentation?

@remi-kazeroni Has your comment been addressed now?

@bouweandela yes, that looks fine for me now. Thanks @SarahAlidoost for taking my comment into account.

@bouweandela bouweandela merged commit 1534899 into main Aug 20, 2021
@bouweandela bouweandela deleted the refactor_era5_cmorizer branch August 20, 2021 12:37
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.

Including recipe_era5.yml under the folder cmorizer is misleading the users.
5 participants