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

XCH4 ESA CMUG diagnostics (subset of the MPQB diagnostics) #1960

Merged
merged 171 commits into from
Jan 26, 2022

Conversation

hb326
Copy link
Contributor

@hb326 hb326 commented Dec 16, 2020

First diagnostic to use ESA CMUG XCH4 data for evaluating CMIP datasets.
Diagnostics are a subset of the C3S MPQB diagnostics, some with minor adjustments.


Checklist for technical review

  • 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)
  • The pull request has a descriptive title that can be used as a one line summary in the changelog
  • The code is composed of functions of no more than 50 lines and uses meaningful names for variables and follows the style guide
  • Documentation is available
  • Please use yamllint to check that your YAML files do not contain mistakes
  • (Only if really necessary) Add any additional dependencies needed for the diagnostic script to setup.py, esmvaltool/install/R/r_requirements.txt or esmvaltool/install/Julia/Project.toml (depending on the language of your script) and also to package/meta.yaml for conda dependencies (includes Python and others, but not R/Julia). Also check that the license of the dependency you want to add and any of its dependencies are compatible with Apache2.0.

New or updated recipe/diagnostic:

  • Documentation for the recipe/diagnostic is available in the doc/sphinx/source/recipes folder and an entry has been added to index.rst
  • Provenance information has been added and no warnings related to provenance are generated when running the recipe

Automated checks pass, status can be seen below the pull request:

  • Circle/CI tests pass. If the tests are failing, click the Details link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • The documentation is building successfully on readthedocs and looks well formatted, click the Details link to see it.

Checklist for scientific review

New or updated recipe/diagnostic:

  • The documentation for the new/updated recipes/diagnostics clearly describes what the recipe does and how to use it
  • The recipe runs successfully on your own machine/cluster or with the @esmvalbot without any modifications to the recipe and with all data specified in the recipe
  • The figure(s) and data look as expected from the literature and/or the paper that is reproduced by the recipe
  • The code contains comments with references to formulas, figures, tables, etc. that are used from papers/online resources

If you need help with any of the items on the checklists above, please do not hesitate to ask by commenting in the issue or pull request.

This pull request needs ESMValCore pull request 1428 (ESMValGroup/ESMValCore#1428) to be merged to be able to run the recipe.

hb326 and others added 6 commits January 21, 2022 10:51
Adding "nh", "sh" and the correct statistics description to the provenance.
Adding "nh" and "sh" to the provenance.
Adjusting the provenance caption as suggested.
@hb326
Copy link
Contributor Author

hb326 commented Jan 21, 2022

  • I have commented in the observational dataset.
  • I cannot think of another reference for these diagnostics. They are very basic in the sense that they are just lineplots without too much statistics behind it...
  • I adjusted the provenance, so that it states next to "global" also "nh" and "sh"
  • It looks to me like "cmug" is already added as project in the provenance. At least it is listed in the provenance record where it is created. Hmm, there might be something going wrong...
  • I changed the caption in the provenance records for the growth to "global mean annual growth rates" as suggested
  • I adjusted the "statistics" for the growth rates plot
  • I added a section to the documentation about the observations and the derivation of the variable

@axel-lauer
Copy link
Contributor

@hb326 Thanks for working hard on this! I think most of the point I raised earlier are fine now. What I still fine a bit strange is the following in the provenance:

  • the attribute 'domains' now always says 'global, northern hemisphere, southern hemisphere' instead of just one of the latter
  • the attribute 'caption' continues to say "global ..." instead of giving the specific region

Other than that, I think all looks good! Nice work!

Copy link
Contributor

@axel-lauer axel-lauer left a comment

Choose a reason for hiding this comment

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

Receipe runs fine, documentation and output look good!

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Cool stuff! 🎉

I only have minor comments regarding the documentation, commented lines and specific plot labels.

doc/sphinx/source/recipes/recipe_mpqb_xch4.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_mpqb_xch4.rst Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/eyring06jgr/eyring06jgr_fig15.ncl Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/eyring06jgr/eyring06jgr_fig15.ncl Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/mpqb/mpqb_cfg_xch4.yml Outdated Show resolved Hide resolved
esmvaltool/recipes/mpqb/recipe_mpqb_xch4.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/mpqb/mpqb_lineplot.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/mpqb/mpqb_lineplot_anncyc.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/mpqb/mpqb_lineplot_growthrate.py Outdated Show resolved Hide resolved
@axel-lauer
Copy link
Contributor

@schlunma Thanks for your review! @hb326 I would recommend to accept all of @schlunma 's proposed changes. Then the PR can be merged. Yay!

@axel-lauer axel-lauer merged commit 7ffb598 into main Jan 26, 2022
@axel-lauer axel-lauer deleted the xch4_esa_cmug_branch branch January 26, 2022 10:29
@hb326
Copy link
Contributor Author

hb326 commented Jan 26, 2022

You were a little quick, @axel-lauer, with the merging. :)
I wanted to update the recipe figure because it is without the observations right now. Will do that in a new pull request then...

@axel-lauer
Copy link
Contributor

@hb326 ups... Sorry about this. I created a pull request with an updated figure including the ESACCI data: #2502
Please take a look and approve PR if you are happy.

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.

6 participants