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

Added recipe, diagnostics and documentation for Schlund et al., ESD (2020) #2015

Merged
merged 20 commits into from Feb 12, 2021

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Feb 5, 2021

This PR adds the recipe, diagnostic scripts and documentation for Schlund et al., ESD (2020).

Since I used this exact code for the analysis in the paper, a thorough scientific review is not necessary. Please note that due to #1903 the recipe needs to be run with the --check-level ignore option.

Depends on #1874.


Before you get started

Checklist

It is the responsibility of the author to make sure the PR is ready to review. The icons indicate whether the item will be subject to the πŸ›  Technical or πŸ§ͺ Scientific review.

New or updated recipe/diagnostic:


To help with the number pull requests:

@schlunma schlunma added this to the v2.2.0 milestone Feb 5, 2021
@schlunma schlunma self-assigned this Feb 5, 2021
@axel-lauer
Copy link
Contributor

@schlunma: I am giving up on trying to install ESMValCore v2.2 for testing. I'll take a look once ESMValCore v2.2 has been released and I succeed in setting up an environment again.

@schlunma
Copy link
Contributor Author

schlunma commented Feb 9, 2021

Absolutely no worries @axel-lauer! I will ping you again when ESMValCore is released and #1874 is merged.

@bettina-gier
Copy link
Contributor

bettina-gier commented Feb 11, 2021

Recipe ran fine, produces the plots from the paper, documentation clear and concise. Provenance runs correctly as well ;)

My only comment would be that

Please note that due to #1903 the recipe needs to be run with the --check-level ignore option.

this should be noted in the documentation or the header of the recipe otherwise the knowledge might get lost down the line before the issue is fixed.

And from the Codacy issues, I guess you could fix this one:
third party import "from esmvalcore.cmor.fixes import add_plev_from_altitude, add_sigma_factory" should be placed before "import esmvaltool.diag_scripts.emergent_constraints as ec"
in esmvaltool/diag_scripts/emergent_constraints/ecs_scatter.py

Also can you rerun the checks? The iris3 env is merged to master now ;)

Co-authored-by: Bettina Gier <gier@uni-bremen.de>
@schlunma schlunma removed the request for review from remi-kazeroni February 11, 2021 14:06
@schlunma
Copy link
Contributor Author

Thanks for the review! 😊

I added the caveat to the documentation.

About the Codacy issue: my local prospector and isort complain if I change the order of that import, so I guess I just leave it as is πŸ˜ƒ

@schlunma
Copy link
Contributor Author

@valeriupredoi would you have time for a brief technical review of this? The vast majority of changes are related to documentation (there's no real new diagnostic here), so this should not take long! Thanks 🍻

@valeriupredoi
Copy link
Contributor

@valeriupredoi would you have time for a brief technical review of this? The vast majority of changes are related to documentation (there's no real new diagnostic here), so this should not take long! Thanks beers

sure, will look at it in 10min πŸ‘

@valeriupredoi
Copy link
Contributor

@schlunma looks good, I didn't read carefully all that documentation, since I trust all is good from a scientific point (I wouldn't know that stuff anyway). Two questions: did you really want to remove the pointers to the seaborn api documentation and could you pls fix that import complaint from Codacy? 🍺

@schlunma
Copy link
Contributor Author

schlunma commented Feb 11, 2021

Thanks for the review!! πŸ‘

  1. The information in the documentation was duplicate since :func:'seaborn.set' already points to the link I removed.
  2. When I change the order my local prospector complains about it and isort automatically changes it back in pre-commit, so not sure what's correct here πŸ˜„

@schlunma schlunma merged commit e5d7720 into master Feb 12, 2021
@schlunma schlunma deleted the schlund20esd branch February 12, 2021 13:20
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.

Add recipe for Schlund et al., ESD (2020)
6 participants