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

Fix provenance of figures of several R diagnostics #2300

Merged
merged 5 commits into from Oct 22, 2021

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Sep 10, 2021

Description

Several R diagnostic scripts made use of the plot_file attribute of the provenance entry of the NetCDF file to log provenance. Unfortunately, this does not work. This pull request fixes the problem by writing a separate provenance record for the plots.

To pass the lintr checks, I also had to modify some variable names to follow the lowercase character/snake case convention from the R style guide and I needed an extra dependency to get litnr to run from pre-commit.


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:

@bouweandela
Copy link
Member Author

@katjaweigel Would you be willing to review this, since you know some R?

@katjaweigel
Copy link
Contributor

ok, I'll have a look.

@katjaweigel katjaweigel self-requested a review September 14, 2021 08:12
@katjaweigel
Copy link
Contributor

@esmvalbot Please run recipe_spei.yml

@esmvalbot
Copy link

esmvalbot bot commented Sep 14, 2021

Since @katjaweigel asked, ESMValBot will run recipe recipe_spei.yml as soon as possible, output will be generated here

@esmvalbot
Copy link

esmvalbot bot commented Sep 14, 2021

ESMValBot is happy to report recipe recipe_spei.yml ran OK, output has been generated here

@katjaweigel
Copy link
Contributor

@bouweandela Looks fine for me, thanks! But there are several other R diagnostics, which would need the same change, so far I found:
esmvaltool/diag_scripts/pv_capacityfactor/pv_capacity_factor.R
and
esmvaltool/diag_scripts/magic_bsc/capacity_factor.R
there it could be solved about the same way.
For esmvaltool/diag_scripts/hyint/*.R it is probably somewhat more difficult.
But probably I should approve this one and then open a new PR for others?

@bouweandela
Copy link
Member Author

I'll have a look, thanks for bringing those to my attention. My R is not good enough to know that there are multiple ways to add keys to a list, so I only looked for the string "plot_file" ;-)

@bouweandela bouweandela changed the title Fix provenance of figures of recipe_spei.yml Fix provenance of figures of several R diagnostics Sep 14, 2021
@bouweandela
Copy link
Member Author

@katjaweigel I updated all occurrences of plot_file I could find in the R diagnostics. At first glance, it looks like the provenance of hyint is implemented without using the problematic plot_file key, but maybe I'm missing something there?

@valeriupredoi
Copy link
Contributor

seems like the newly requested R package has made an already very slow conda env update to go even slower, dis not gud, amigo 🦙

@katjaweigel
Copy link
Contributor

@bouweandela I think hyint uses functions stored in other scripts (e.g. hyint_plot_trends.R) to do the plots, but they don't write any provenance. But I think it is possible (or already part of the data given back), to get the file name of the figure back from these functions, to write the provenance file for all figures where the other provenance file is written. But I'm not really sure about this and have to check it in more detail.

@bouweandela
Copy link
Member Author

@esmvalbot Please run recipe_hyint.yml

@esmvalbot
Copy link

esmvalbot bot commented Sep 16, 2021

Since @bouweandela asked, ESMValBot will run recipe recipe_hyint.yml as soon as possible, output will be generated here

@bouweandela
Copy link
Member Author

From just looking at the code of the hyint diagnostics, provenance looked OK, but let's try running the recipes to be sure.

@bouweandela bouweandela added this to the v2.4.0 milestone Sep 16, 2021
@esmvalbot
Copy link

esmvalbot bot commented Sep 16, 2021

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

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.

OK by me from a purely technical (environment/testing) point of view, but please merge only when the sciencey bit is reviewed 👍

@bouweandela
Copy link
Member Author

There is no science bit to this pull request, it's only a bugfix for the provenance recording procedure.

@zklaus
Copy link
Contributor

zklaus commented Oct 22, 2021

@bouweandela, fair enough. Could you merge the latest main to check the right tests?

@bouweandela
Copy link
Member Author

Done

@zklaus zklaus merged commit 9e35223 into main Oct 22, 2021
@zklaus zklaus deleted the fix-provenance-of-figures-in-recipe_spei.yml branch October 22, 2021 14:36
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

4 participants