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

Recipe to reproduce the 2014 KNMI Climate Scenarios (kcs). #1667

Merged
merged 89 commits into from Sep 24, 2020

Conversation

Peter9192
Copy link
Contributor

@Peter9192 Peter9192 commented May 27, 2020

This recipe reproduces the basic steps described in Lenderink 2014, used at the time to produce climate scenario's for the Netherlands. A recent re-implementation of the code lives here, but we figured that porting the code to ESMValTool would make the code more maintainable and sustainable.

An initial attempt at porting the code lives here, but it can benefit much more from the built-in functionality in ESMValTool and -Core. Therefore we decided to start from scratch in this PR, while keeping a link to the older branch for future reference.


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)
  • Give this pull request a descriptive title that can be used as a one line summary in a changelog
  • 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

New recipe/diagnostic

  • Add documentation for the recipe to the doc/sphinx/source/recipes folder and add a new entry to index.rst
  • Add provenance information

@bouweandela bouweandela added diagnostic EUCP www.eucp-project.eu labels Jun 8, 2020
@Peter9192
Copy link
Contributor Author

Hi @karinvdwiel, welcome and thanks for the suggestions! I implemented all of them. I attached the updated figure. For the next time, I can show you how to preview the docs on your own computer.

global_matching

…sing numpy indexing rather than xarray labelled indexing).
@Peter9192
Copy link
Contributor Author

The performance of the second diagnostic scales exponentially with the number of ensemble members for the target model. This led to very long execution time for more than, say, 8 members. I made this step a bit more efficient, which leads to a substantial speedup (~60x). I also discovered a bug when using more than 9 ensemble members, which is now fixed as well.

@karinvdwiel, if you are happy with the diagnostic, could you approve the pull request from a scientific point of view? And @bouweandela, could you do a technical check?

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.

Looks good! I'll continue reviewing tomorrow.

doc/sphinx/source/recipes/recipe_kcs.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_kcs.rst Show resolved Hide resolved
esmvaltool/config-references.yml Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/kcs/global_matching.py Outdated Show resolved Hide resolved
esmvaltool/recipes/recipe_kcs.yml 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.

Looks good! Just some minor suggestions for improvement.

esmvaltool/recipes/recipe_kcs.yml Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/kcs/local_resampling.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/kcs/local_resampling.py 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.

You were right @Peter9192, it is indeed a bit more involved to clean up the run_dir. Don't worry about breaking other diagnostics though, the only other Python diagnostic that uses it is esmvaltool/diag_scripts/cvdp/cvdp_wrapper.py.

esmvaltool/diag_scripts/shared/_base.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/shared/_base.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/shared/_base.py 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.

@mattiarighi Could you please do a final scientific review and test?

@Peter9192 Peter9192 mentioned this pull request Aug 21, 2020
10 tasks
@Peter9192
Copy link
Contributor Author

@axel-lauer this PR has been waiting for a final round of testing and scientific review (it has already been screened by @karinvdwiel). Is it okay if we merge this, or would you like another review by someone from the scientific team?

@nielsdrost
Copy link
Member

BEEP this is the manual ESMValBot BEEP

I've ran the recipe "recipe_kcs.yml" in this branch on the ESMValTool test machine. The output is available here:

https://esmvaltool.cloud.dkrz.de/shared/esmvaltool/kcs-test/output/recipe_kcs_20200916_125334

@axel-lauer @karinvdwiel could you have a look at the output and see if this PR is now ready for merge?

Thanks!

@bouweandela
Copy link
Member

Merging this now as a scientific review has been done by @karinvdwiel from KNMI.

@bouweandela bouweandela merged commit 178e4f7 into master Sep 24, 2020
1 check failed
@bouweandela bouweandela deleted the lenderink2014 branch September 24, 2020 11:10
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.

None yet

5 participants