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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Fig. 6, 7 and 9 of Bock20jgr #2252

Merged
merged 36 commits into from
Oct 14, 2021
Merged

Add Fig. 6, 7 and 9 of Bock20jgr #2252

merged 36 commits into from
Oct 14, 2021

Conversation

LisaBock
Copy link
Contributor

@LisaBock LisaBock commented Aug 5, 2021

Description

Add Fig. 6, 7 and 9 of Bock et al., JGR, 2021.

Fig. 6: Performance metrics for 17 atmospheric variables
Fig. 7: Correlation Pattern for 5 atmospheric variables
Fig. 9: Annual mean shortwave cloud radiative effect (map + zonal mean)


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:

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.

I tested the following three recipes

  • recipe_bock20jgr_fig_1-4: runs OK, output looks fine
  • recipe_bock20jgr_fig_6-7: crashes, see below
  • recipe_bock20jgr_fig_8-10: runs with warnings (see below), output looks fine

recipe_bock20jgr_fig_6-7

Recipe produces successfully the "perfmetrics" plot (fig. 6) but the NCL script producing fig. 7 (bock20jgr/corr_pattern_collect.ncl) crashes with the following error:

INFO    warning: in cor_collect, specified order of diagnostics cannot be applied, invalid entry in diag_order
fatal:Subscript out of range, error in subscript #1
fatal:An error occurred reading data_all
fatal:["Execute.c":8637]:Execute: Error occurred at or near line 177 in file /mnt/lustre02/work/bd0854/b380103/ESMValTool/esmvaltool/diag_scripts/bock20jgr/corr_pattern_collect.ncl

recipe_bock20jgr_fig_8-10

Recipe produces the following noteworthy warnings that should probably be checked:

  • task fig_8_ecs_cmip3/ecs: WARNING [25727] Invalid ancestor file [...]/ESMValTool/esmvaltool/diag_scripts/climate_metrics/external_sources/ipcc_ar4.yml specified for recording provenance of [...]/recipe_bock20jgr_fig_8-10_20210816_071207/work/fig_8_ecs_cmip3/ecs/ecs.nc, created by diagnostic script climate_metrics/ecs.py in task fig_8_ecs_cmip3/ecs
  • task fig_10_cmip6/feedback_parameters: WARNING [25727] The reference file [...]/ESMValTool/esmvaltool/references/andrews12grl.bibtex does not exist, citation information incomplete.

Other comments / suggestions

  • I would recommend to add the figure numbers of Bock et al. (2020) to the captions of the example figures in the documentation. This would make it easier to identify the figures as the documentation uses the original figure numbers from the paper.
  • I would strongly discourage to disable the provenance output in esmvaltool/diag_scripts/clouds/clouds_ipcc.ncl (lines 763-764). Writing too large provenance records to .png files is known to cause the ESMValTool to crash (see e.g. Test esmvalcore=2.3.0 with current batch of ESMValTool recipes聽#2198 (comment)). As a work-around until this problem is fixed, .pdf output can be used.

@LisaBock
Copy link
Contributor Author

@schlunma Could you please check the comments by Axel regarding your figures 8 and 10? (By the way I divided the recipe in three pieces, otherwise the running time was too long.) Thanks!

@LisaBock
Copy link
Contributor Author

@axel-lauer Thanks for reviewing!
Manuel and I worked on your requests. Could you please run once more the recipe recipe_bock20jgr_fig_6-7.yml? It is not running for me but I am not sure if it is a general problem or just mine... Thanks!

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.

I retested recipe_bock20jgr_fig_6-7. The recipe runs successfully and the output looks fine. I noticed that in contrast to the original publication there are only 14 CMIP3 models included in figs. 6+7 instead of 19. I presume that was intentional? If so, then looks all good now!

Copy link
Contributor

@remi-kazeroni remi-kazeroni 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 all the work @LisaBock! I ran the 3 recipes and everything works fine: plots produced, provenance generated, runs successfull... But this works only with the stable version of the Core. The recipes in their current state won't run with the next Core release. You would need the following:

For the first 2 points, I think it would be better to fix things in this PR. My other comments are just related to minor issues or suggestions for the documentation.

doc/sphinx/source/recipes/recipe_bock20jgr.rst Outdated Show resolved Hide resolved
collect:
description: Wrapper to collect and plot previously calculated metrics
scripts:
RMSD:
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to rename this script fig_6_RMSD to make it easier to find the corresponding figure and data when looking at the ESMValTool run.

@@ -1,17 +1,16 @@
# ESMValTool
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only none yamllint compliant recipe of the PR. I leave it up to you to say if this would help readability of this recipe or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would leave it as it is...

esmvaltool/diag_scripts/bock20jgr/corr_pattern.ncl Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/bock20jgr/corr_pattern_collect.ncl Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/bock20jgr/corr_pattern_collect.ncl Outdated Show resolved Hide resolved
@LisaBock
Copy link
Contributor Author

Thanks for the technical review @remi-kazeroni !

I responded to your comments and also work on the two additional issues you mentioned (titles, write_plots).

I think it's ready for merging...

@zklaus
Copy link
Contributor

zklaus commented Oct 12, 2021

@LisaBock, please also take care of the checklist in the summary. We generally expect authors to tick all applicable boxes and strike out those that don't apply before the review.

@LisaBock
Copy link
Contributor Author

@LisaBock, please also take care of the checklist in the summary. We generally expect authors to tick all applicable boxes and strike out those that don't apply before the review.

@zklaus I thought this is the task for the reviewer. Now I checked again the checklist and ticked off all issues.

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, thanks for the changes @LisaBock!

@remi-kazeroni
Copy link
Contributor

Thanks for the technical review @remi-kazeroni !

I responded to your comments and also work on the two additional issues you mentioned (titles, write_plots).

I think it's ready for merging...

You're welcome @LisaBock! Unless @axel-lauer wants to have a final look, I would say that this is good to merge

@zklaus
Copy link
Contributor

zklaus commented Oct 12, 2021

@LisaBock, I know there is some confusion and we need to improve our documentation on that point. Thanks for taking care of this!

@LisaBock
Copy link
Contributor Author

I do have the approved technical review by @remi-kazeroni and the aprroved scientific review by @axel-lauer . Could you please merge it now? Or is something missing?

@axel-lauer axel-lauer merged commit c56f8d2 into main Oct 14, 2021
@axel-lauer axel-lauer deleted the bock20jgr branch October 14, 2021 07:24
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.

None yet

5 participants