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

PV capacity factor for ESMValTool GMD paper #2153

Merged
merged 20 commits into from Jul 27, 2021
Merged

PV capacity factor for ESMValTool GMD paper #2153

merged 20 commits into from Jul 27, 2021

Conversation

katjaweigel
Copy link
Contributor

@katjaweigel katjaweigel commented May 4, 2021

Description

PV capacity factor described in GMD paper https://gmd.copernicus.org/preprints/gmd-2020-244/


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:

@katjaweigel
Copy link
Contributor Author

Documentation is still missing and provenance needs to be checked/added.

@katjaweigel katjaweigel marked this pull request as ready for review May 26, 2021 09:12
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.

Hi @katjaweigel, thanks for your nice work here! I did a technical review and left some comments about styling and documentation. I'm not fluent in R but your scripts look good to me. Feel free to take into account the styling issues I found.

Nevertheless, I can't mark this PR approved because I couldn't run the recipe with the latest ESMValCore 2.3.0. I got an issue with the multi_model_statistics (see log attached).

@zklaus: even though this PR is not yet in the main branch, would this count as another test of the ESMValCore 2.3.0 like in #2198?
main_log_debug.txt

doc/sphinx/source/recipes/recipe_pv_capacity_factor.rst Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/pv_capacityfactor/PV_CF.R Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/pv_capacityfactor/PV_CF.R Outdated Show resolved Hide resolved
@katjaweigel
Copy link
Contributor Author

Dear @remi-kazeroni thanks for your comments and corrections! I'll go through them and test, what happens if I run it with the currents Core and if I can help to solve this.

@zklaus
Copy link
Contributor

zklaus commented Jun 18, 2021

@zklaus: even though this PR is not yet in the main branch, would this count as another test of the ESMValCore 2.3.0 like in #2198?
main_log_debug.txt

Sure! Though if it doesn't make its way into ESMValTool 2.3.0 and requires some change to the core that doesn't impact anything else, that change might be relegated to ESMValCore 2.4.0.

@zklaus
Copy link
Contributor

zklaus commented Jun 18, 2021

That said, @remi-kazeroni, if you think the changes needed in this PR are relatively minor, feel free to assign milestone 2.3.0 and we'll do our best to get it in.

@katjaweigel
Copy link
Contributor Author

@remi-kazeroni sorry, I didn't see the other ones, they were hidden behind some button "Hidden conversions" and I thought it would only hide resolved one. I'll look at them soon (need to leave soon, so I guess I can't finish it now).

@katjaweigel
Copy link
Contributor Author

@remi-kazeroni sorry, I didn't see the other ones, they were hidden behind some button "Hidden conversions" and I thought it would only hide resolved one. I'll look at them soon (need to leave soon, so I guess I can't finish it now).

@remi-kazeroni I hope I found and corrected everything this time.

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 @katjaweigel! Everything looks good to me now from the technical side 👍

@Emmadd
Copy link
Contributor

Emmadd commented Jul 6, 2021

Hi, I'll try to review this request, but it might be a bit difficult to get input data to actually run the recipe locally. @remi-kazeroni is the output of your successful run somewhere, so I can see that instead?

@katjaweigel
Copy link
Contributor Author

katjaweigel commented Jul 7, 2021

Hi, I'll try to review this request, but it might be a bit difficult to get input data to actually run the recipe locally. @remi-kazeroni is the output of your successful run somewhere, so I can see that instead?

@Emmadd thanks for reviewing! Due to the discussion about the multi-model mean for daily data I'll completely remove it for the moment. Without multi-model mean the recipe should actually run on any data set which contains the variables tas and rsds (many CMIP5 or 6 models, ERA-Interim). I ran it on Mistral, if you can access this, the latest output with the setting above is at:
/work/bd1083/b380216/output/recipe_pv_capacity_factor_20210629_071411

/work/bd1083/b380216/output/recipe_pv_capacity_factor_20210707_075819/ with most recent changes.

@Emmadd
Copy link
Contributor

Emmadd commented Jul 12, 2021

@esmvalbot please run recipe_pv_capacity_factor.yml

@esmvalbot
Copy link

esmvalbot bot commented Jul 12, 2021

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

@esmvalbot
Copy link

esmvalbot bot commented Jul 12, 2021

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

Copy link
Contributor

@Emmadd Emmadd left a comment

Choose a reason for hiding this comment

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

Hi, the code looks ok to me. A few things confused me where wind is (still) mentioned, I would like to see those addressed. ESMValbot was able to run the recipe, but I haven't been able to see and compare the figure(s). Best, Emma

doc/sphinx/source/recipes/recipe_pv_capacity_factor.rst Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/pv_capacityfactor/PV_CF.R Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/pv_capacityfactor/PV_CF.R Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/pv_capacityfactor/PV_CF.R Outdated Show resolved Hide resolved
esmvaltool/recipes/recipe_pv_capacity_factor.yml Outdated Show resolved Hide resolved
@katjaweigel
Copy link
Contributor Author

Hi, the code looks ok to me. A few things confused me where wind is (still) mentioned, I would like to see those addressed. ESMValbot was able to run the recipe, but I haven't been able to see and compare the figure(s). Best, Emma

@ thanks @Emmadd ! The figures are there: https://esmvaltool.cloud.dkrz.de/shared/esmvaltool/esmvalbot-output/pvcf_gmddraft-kr0c7llt/recipe_pv_capacity_factor_20210712_081803/plots/capacity_factor/main/
This really should be improved that this link is not given on the esmvalbot output page but one needs to put it together from its link and the main_log.txt

@Emmadd
Copy link
Contributor

Emmadd commented Jul 13, 2021

Figures look fine!

@zklaus zklaus added this to the v2.4.0 milestone Jul 13, 2021
@katjaweigel
Copy link
Contributor Author

katjaweigel commented Jul 14, 2021

@Emmadd I tried to address all points you wrote in the review, can you check if this is ok now?
The main partly open points are, if I should show a model and an ERA-Interim Figure in the docu and if it is more important to keep the path name as in the paper or the consistency between the path and the recipe/diagnostic?

@Emmadd
Copy link
Contributor

Emmadd commented Jul 16, 2021

Hi @katjaweigel changes look good to me. It's fine to stick with the IPSL figure. As for the path name I'd prefer consistency between the recipe/diagnostic, but that's just my humble opinion.

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.

PV capacity factor for ESMValTool GMD paper
5 participants