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

Update Climwip recipe to reproduce brunner2020esd #1859

Merged
merged 51 commits into from
Jun 23, 2021
Merged

Conversation

ruthlorenz
Copy link
Contributor

@ruthlorenz ruthlorenz commented Oct 14, 2020

Before you start, please read our contribution guidelines.

Please discuss your idea with the development team before getting started, to avoid disappointment later. The way to do this is to open a new issue on GitHub. If you are planning to modify an existing functionality, please discuss it with the original author(s) by tagging them in the issue.


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

Closes #1850

@ruthlorenz ruthlorenz changed the title Climwip brunner2020esd updating Climwip recipe to reproduce brunner2020esd Nov 3, 2020
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Hi @ruthlorenz this looks like a great start! I highlighted some points, let me know if you need more help :-)

@bouweandela bouweandela added diagnostic EUCP www.eucp-project.eu labels Feb 9, 2021
@ruthlorenz ruthlorenz marked this pull request as ready for review April 26, 2021 10:25
@lukasbrunner
Copy link
Contributor

Hi @ruthlorenz @lukasbrunner Nice work! I made some comments on the code to simplify it a little bit here and there, see what you think.

Other than that, I cannot approve it yet because documentation is missing. Could you add a description explaining what the recipe does and how it can be used? Even if it is not much different from the previous version, the new recipe: recipe_climwip_brunner20esd.yml should be referenced in the documentation. Maybe this is a good location: https://esmvaltool--1859.org.readthedocs.build/en/1859/recipes/recipe_climwip.html

Thanks a lot for the helpful comments @stefsmeets ! I've resolved all of them. The only open issue is the documentation. You are right of course it is missing. This is because we planed to merge #2109 first. There I included a major update of the documentation, which is already through scientific review. But that PR is waiting for a CI issue to be resolved in with a core update, so I guess now this one needs to wait too...

@stefsmeets
Copy link
Contributor

stefsmeets commented May 21, 2021

Thanks a lot for the helpful comments @stefsmeets ! I've resolved all of them. The only open issue is the documentation. You are right of course it is missing. This is because we planed to merge #2109 first. There I included a major update of the documentation, which is already through scientific review. But that PR is waiting for a CI issue to be resolved in with a core update, so I guess now this one needs to wait too...

Yeah, that's annoying, I have also run into this in the past. Luckily the new release is imminent (2021-06-14 for esmvalcore), so the wait is not that long 😅

If this PR depends on #2109, you can merge that into this branch (git merge add_recipe_climwip). Then the documention is here as well, and you can make the necessary changes so that this one is ready to be merged too as soon as esmvalcore 2.3 gets released. The esmvaltool release is 2 weeks after 👍

Meanwhile, there are still some flake8 errors to fix:

/test/esmvaltool/diag_scripts/weighting/weighted_temperature_map.py:66:1: W293 blank line contains whitespace
/test/esmvaltool/diag_scripts/weighting/weighted_temperature_map.py:75:1: W293 blank line contains whitespace

@lukasbrunner
Copy link
Contributor

@stefsmeets I've now merged the updated documentation from #2109 and added some recipe specific parts. If you could have another quick look at approve it that would be great.

@stefsmeets stefsmeets self-requested a review June 22, 2021 11:26
Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

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

Thanks for adding the documentation @lukasbrunner ! 🚀

@lukasbrunner
Copy link
Contributor

@ESMValGroup/esmvaltool-coreteam This PR is ready to be merged. Sorry for the not so clean separation between contributor and scientific reviewer. The PR was mainly done by @ruthlorenz and reviewed by me. I only added the updated documentation.

@zklaus zklaus requested a review from Peter9192 June 22, 2021 16:02
@zklaus
Copy link
Contributor

zklaus commented Jun 22, 2021

@Peter9192, since there were formally outstanding comments from your side, could you please give your +/-1?

@Peter9192
Copy link
Contributor

@Peter9192, since there were formally outstanding comments from your side, could you please give your +/-1?

I provided some suggestions when this was still a draft, I think those things have long been adressed :-)

@Peter9192
Copy link
Contributor

@zklaus

@zklaus zklaus merged commit 5d29418 into main Jun 23, 2021
@zklaus zklaus deleted the climwip_brunner2020esd branch June 23, 2021 12:40
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.

Updating ClimWIP recipe to reproduce Brunner et al. 2020 ESD (CMIP6, more variable groups including trend)
6 participants