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

AutoAssess: Add new diagnostic for radiation budget #2282

Merged
merged 43 commits into from
Dec 2, 2021

Conversation

Jon-Lillis
Copy link
Contributor

@Jon-Lillis Jon-Lillis commented Sep 3, 2021

Description


Before you get started

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


To help with the number of pull requests:

@Jon-Lillis Jon-Lillis self-assigned this Sep 3, 2021
@Jon-Lillis
Copy link
Contributor Author

@esmvalbot Please run recipe_radiation_budget.yml

@esmvalbot
Copy link

esmvalbot bot commented Sep 3, 2021

Since @Jon-Lillis asked, ESMValBot will run recipe recipe_radiation_budget.yml as soon as possible, output will be generated here

@esmvalbot
Copy link

esmvalbot bot commented Sep 3, 2021

ESMValBot is sorry to report it failed to run recipe recipe_radiation_budget.yml: exit is 1, output has been generated here

@Jon-Lillis
Copy link
Contributor Author

Jon-Lillis commented Oct 20, 2021

Moved plots to issue #2209.

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

all good from me, cheers and good work, guys! All we need now is a sciencey reviewer - @alistairsellar when you gots some time, mate 🍺

Copy link
Contributor

@alistairsellar alistairsellar left a comment

Choose a reason for hiding this comment

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

I'm happy from a science perspective. The values in the plots attached to #2209 appear identical to the eye, and the metrics in the csv files differ by less than 0.75%, and only 0.06% on average. The science parts of the code look to be doing the right thing.

#2209 notes that there are some variables missing in the CERES files in the data pool and that this will be picked up in a later issue. Fine by me - please open that issue now so we don't lose sight of the gap.

Copy link
Contributor

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Thanks everyone!

I believe the only outstanding issues (not related to any of my comments) is the general principle on whether diagnostics should fail or warn when inputs are missing.

doc/sphinx/source/recipes/recipe_radiation_budget.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_radiation_budget.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_radiation_budget.rst Outdated Show resolved Hide resolved
Comment on lines +387 to +403
def get_provenance_record(filenames):
"""Return a provenance record describing the plot.

Parameters
----------
filenames : list of strings
The filenames containing the data used to create the plot.

Returns
-------
dictionary
The provenance record describing the plot.
"""
record = {
'ancestors': filenames,
}
return record
Copy link
Contributor

Choose a reason for hiding this comment

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

We added a bare minimum provenance record. This is all we needed to add for the recipe to run without errors. The tutorial has a very short "Recording the provenance" section, which links to the ESMValTool documentation, but we weren't sure on the best practice here. Is there anything else that we should add here? Would it be worth adding more details to the tutorial about this?

esmvaltool/recipes/recipe_radiation_budget.yml Outdated Show resolved Hide resolved
Jon-Lillis and others added 6 commits November 23, 2021 10:54
Co-authored-by: Emma Hogan <ehogan@users.noreply.github.com>
Co-authored-by: Emma Hogan <ehogan@users.noreply.github.com>
Co-authored-by: Emma Hogan <ehogan@users.noreply.github.com>
Co-authored-by: Emma Hogan <ehogan@users.noreply.github.com>
Co-authored-by: Emma Hogan <ehogan@users.noreply.github.com>
Co-authored-by: Emma Hogan <ehogan@users.noreply.github.com>
Copy link
Contributor

@ehogan ehogan 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 making those changes, @Jon-Lillis! :)

@ehogan
Copy link
Contributor

ehogan commented Dec 2, 2021

@valeriupredoi I think this PR is ready to be merged 🎉 Is there anything else we need to do before this happens?

@valeriupredoi
Copy link
Contributor

awesome job here, guys! 🍺

@valeriupredoi valeriupredoi merged commit ce6092c into main Dec 2, 2021
@valeriupredoi valeriupredoi deleted the radiation_budget_new_diagnostic branch December 2, 2021 16:55
@valeriupredoi
Copy link
Contributor

one last question, from @alistairsellar

#2209 notes that there are some variables missing in the CERES files in the data pool and that this will be picked up in a later issue. Fine by me - please open that issue now so we don't lose sight of the gap.

Has this been opened? 🍺

@Jon-Lillis
Copy link
Contributor Author

Yep, this was opened in #2428. I've modified the description to include adding the additional variables to the radiation budget diagnostic.

@valeriupredoi
Copy link
Contributor

excellent, cheers @Jon-Lillis 🍺

@ehogan ehogan added the AutoAssess Issues relevant to the conversion of AutoAssess metrics from Met Office to ESMValTool label Dec 10, 2021
@ehogan ehogan changed the title Radiation budget new diagnostic AutoAssess: Add new diagnostic for radiation budget Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved by scientific reviewer approved by technical reviewer AutoAssess Issues relevant to the conversion of AutoAssess metrics from Met Office to ESMValTool diagnostic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AutoAssess: Add new diagnostic for radiation budget
6 participants