search

RMI-PACTA / pacta.executive.summary

Repo containing the code to generate charts in the PACTA COP executive summary document.
https://rmi-pacta.github.io/pacta.executive.summary/
Other
2 stars 2 forks source link

338 align nz commitment sbti #340

Closed Antoine-Lalechere closed 2 months ago

Antoine-Lalechere commented 2 months ago

solves #338

This PR aims to compute Net Zero Commitments for the PACTA COP Switzerland 2024 project. We want to add a second way to compute the aggregate score to comply the SBTI requirement.

If a portfolio has only one companies out of 10 setting targets, but that company represent 2% of the portfolio, last year the portfolio was having a 10% commitment to Net Zero, and the new indicator will only provide a 2% commitment to Net Zero.

This PR is still draft as peer group hasn't been added.

prep_net_zero_commitments output earns one column indicator with value being number_company (as delivered last exercice) or exposure_share (as SBTI compliant)

image
codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.

Project coverage is 9.58%. Comparing base (6853956) to head (2d7669b).

Files Patch % Lines
R/prep_net_zero_commitments.R 0.00% 35 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #340 +/- ## ======================================== - Coverage 9.69% 9.58% -0.11% ======================================== Files 27 27 Lines 2187 2212 +25 ======================================== Hits 212 212 - Misses 1975 2000 +25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MonikaFu commented 2 months ago

@Antoine-Lalechere if it is still a draft can you please convert it to draft?

Antoine-Lalechere commented 2 months ago

Hi Monika, this PR is ready for review This PR includes net zero commitment indicators computed as it was done in 2022 and as it should to be compliant with SBTI. This PR also adds peers results.

prep_net_zero_commitments will now output one additional column indicator which specify if the indicator is compliant with SBTI requirement (exposure_share) or with what was delivered last year (number_company)

I included a file with some random figures for peers comparison here, not sure it will be appropriate when we'll use real data but this is unrelated to Swiss value currently @MonikaFu

Antoine-Lalechere commented 2 months ago

@MonikaFu @AlexAxthelm

PR has been pushed without the file I just put in the other file Peers functions has been grouped in one only function and one only output.

Output formatting for the function has been changed and look now as this - I think it's harder to get than what we had before but if you think it's better for the next step let's keep it this way

image
Antoine-Lalechere commented 2 months ago

actually it looks like this so that we don't have different column name

image
Antoine-Lalechere commented 2 months ago

changing the 2 comments now

About the data-raw file, where should I then put the function that create the peer group file?

MonikaFu commented 2 months ago

changing the 2 comments now

About the data-raw file, where should I then put the function that create the peer group file?

@Antoine-Lalechere I'd say since we're planning to do the calculation in workflow.prep.PA2024CH we should put the file there, no?

MonikaFu commented 2 months ago

Here we only read in the pre-calculate share values for peers which will be saved in user_data\score_card if I understand correctly. @AlexAxthelm what do you think?

AlexAxthelm commented 2 months ago

Here we only read in the pre-calculate share values for peers which will be saved in user_data\score_card if I understand correctly. @AlexAxthelm what do you think?

I haven't checked the code to verify that's accurate, but that sounds like what we discussed today.

MonikaFu commented 2 months ago

@Antoine-Lalechere I think accidentally some changes were reverted. Please, have a look at the code in this PR. I do remember seeing you making the changes requested but now I don't see them.

Antoine-Lalechere commented 2 months ago

I think the last change I made are still there? @MonikaFu

MonikaFu commented 2 months ago

image this is what I see in the current state of the PR. The parameter name for financial data with sbti is still called 'net_zero_targets' while in commit #12bbbb4 is it changed to 'fin_data_net_zero_targets' as requested.

Antoine-Lalechere commented 2 months ago

clear now, I'll change it @MonikaFu @AlexAxthelm I don't get what you did, what should I do to push some new change, I should pull first from the package?

AlexAxthelm commented 2 months ago

@AlexAxthelm I don't get what you did, what should I do to push some new change, I should pull first from the package?

I haven't touched this PR yet. is there something for me to do on this one?

Antoine-Lalechere commented 2 months ago
image

I'm confused by this then

Antoine-Lalechere commented 2 months ago

clear now @MonikaFu

AlexAxthelm commented 2 months ago

Noting that the R CMD Check action is issuing a WARNING for code/documentation mismatch in prep_net_zero_commitments()

https://github.com/RMI-PACTA/pacta.executive.summary/actions/runs/9642383044/job/26590061228?pr=340#step:6:144

Also a NOTE about the large extdata directory, but that's out of scope for this PR (See #336)

Antoine-Lalechere commented 2 months ago

Thanks @AlexAxthelm for the suggested commit - I also changed my RStudio to be aligned with yours and team's @MonikaFu

MonikaFu commented 2 months ago

Thanks @AlexAxthelm for the suggested commit - I also changed my RStudio to be aligned with yours and team's @MonikaFu

right! I guess we should actually gitignore .Rproj but that is for another issue.

MonikaFu commented 2 months ago

@Antoine-Lalechere can you please re-document the code as checks seem to be failing sure to some arguments not being documented?

AlexAxthelm commented 2 months ago

@MonikaFu @Antoine-Lalechere, it looks like merging this in has broken the nightly build for workflow.transition.monitor. Was PR this tested against that workflow?

More importantly, do we have a PR in prep that can adjust to the new signatures?

AlexAxthelm commented 2 months ago

@Antoine-Lalechere @MonikaFu @cjyetman This change in ES is still causing issues in workflow.transition.monitor. I think we need to change some of the arguments being called somewhere, but I haven't lookd if we need to change them in web_tool_script_3, or somewhere else.

https://github.com/RMI-PACTA/workflow.transition.monitor/actions/runs/9736044135/job/26867073761#step:10:108

You can see that this is affecting all tests that generate an ES:

https://github.com/RMI-PACTA/workflow.transition.monitor/actions/runs/9736044135

cjyetman commented 2 months ago

the error...

Quitting from lines 193-218 [scorecard] (./scorecard.Rmd) Error in prep_net_zero_commitments(): ! unused arguments (net_zero_targets = fin_data_net_zero_targets, peer_group_share_net_zero = peer_group_share_net_zero) Execution halted

Antoine-Lalechere commented 2 months ago

Ho yes that's right I change the parameter name peer_group_share_net_zero to peers_net_zero_commitment to make it clearer as the PR aim to change how the share was computed

MonikaFu commented 2 months ago

@AlexAxthelm I believe that the changes in these two PRs : https://github.com/RMI-PACTA/pacta.executive.summary/pull/346 and https://github.com/RMI-PACTA/workflow.transition.monitor/pull/331 plus the changes in testing set-up I told you about should solve this issue. Sorry for merging this PR in before making everything fully functional. I somehow thought that this one was just adding a new function that was not used anywhere yet. Clearly I was wrong.