-
Notifications
You must be signed in to change notification settings - Fork 0
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
config: point pa2024ch
to data prep with new weo 2023 scenarios
#333
Conversation
The newly created data directory now contains APS and STEPS scenarios for WEO 2023 for the aviation, cement and steel sectors. With these scenarios missing, the executive summary couldn't render correctly. Relates to: RMI-PACTA/workflow.data.preparation/pull/238 RMI-PACTA/workflow.data.preparation/pull/237 RMI-PACTA/workflow.data.preparation/pull/236 RMI-PACTA/pacta.scenario.data.preparation/pull/61 RMI-PACTA/pacta.scenario.data.preparation/pull/63
Open question to @AlexAxthelm and @cjyetman do you think I should update the It might make life less confusing down the road to keep both initiatives with the same data version. |
I think that we have said that whatever happens with WEO will happen everywhere (PA2024CH, GENERAL, and all 2023Q4 initiatives), and I expect that word has traveled around, so I think we should follow through on that. Additionally, until this issue is resolved, I prefer that we don't get into using different 2023Q4 datasets for different initiatives: |
Cool! done |
Going to wait for checks to pass and inspect the outputs prior to merging |
presumably if #331 merges and this PR branch is updated, the tests will pass? so we're waiting on that, no? |
Indeed, depends on #331 I will re-trigger CI/CD once that is merged. |
Docker build status
|
shit! |
2024-07-02 13:16:23.402782 ES: There was an error in prep/plot_scores(). Returning empty plot object.
2024-07-02 13:16:23.616256 ES: There was an error in prep/plot_scores(). Returning empty plot object. Which is actually super bizarre since that isn't even the expected error message... This might be a cache invalidation thing. |
I think you might be looking at the wrong lines. The message you're getting is at line 354 & 377 |
and don't have anything interesting on them? Are we talking about different files? |
@MonikaFu FYI, the problem seems to be that some of the sectors yield data_scores_b <- prep_scores(
results_portfolio = results_portfolio,
peers_results_aggregated = peers_results_aggregated,
asset_class = "bonds",
scenario_source = scenario_source
) I get: or # A tibble: 14 × 5
asset_class scope entity sector score
<chr> <chr> <chr> <chr> <chr>
1 bonds portfolio peers NA E
2 bonds sector peers automotive C
3 bonds sector peers aviation NA
4 bonds sector peers cement NA
5 bonds sector peers coal E
6 bonds sector peers gas E
7 bonds sector peers oil E
8 bonds sector peers power E
9 bonds sector peers steel NA
10 bonds portfolio this_portfolio NA A+
11 bonds sector this_portfolio gas A
12 bonds sector this_portfolio oil C
13 bonds sector this_portfolio power A+
14 bonds sector this_portfolio steel E This seems to be what is causing the plot error. |
This is the error I get: Error in `abort_if_invalid_values()` at pacta.executive.summary/R/plot_scores.R:227:3:
! Each value of `"sector"` must be one of these:
NA, power, automotive, coal, oil, gas, aviation, steel.
✖ You passed: cement.
Run `rlang::last_trace()` to see where the error occurred. |
@MonikaFu unless there is a specific reason not to? Relates to RMI-PACTA/workflow.transition.monitor#333 and in particular RMI-PACTA/workflow.transition.monitor#333 (comment)
Probably solved by RMI-PACTA/pacta.executive.summary#348 |
* bug: allow `cement` as an input sector to `plot_scores` @MonikaFu unless there is a specific reason not to? Relates to RMI-PACTA/workflow.transition.monitor#333 and in particular RMI-PACTA/workflow.transition.monitor#333 (comment) * bump snaps
@AlexAxthelm @cjyetman @MonikaFu What that means we need to re-run the peer data (might already have been done by @AlexAxthelm , need to make sure the latest run contains STEPS, APS and NZE for the aviation, cement and steel sectors) and then either:
This runs in parallel to a discussion we've already been having about updating our peer data in the CI/CD pipeline. Converting this PR to draft until the above has been completed. |
Latest bump of data_share_path now incorporates changes from RMI-PACTA/pacta.data.preparation#32 |
For the record: after a call with @AlexAxthelm we realized that we need the appropriate versions of To that end, I have asked him to target #333 with the #340 PR, and will merge it that together with this PR (I will update the PR title and description appropriately). I will also wait until @MonikaFu runs and updates the After all the above is complete, we should have a good idea of if all IR and ES building runs well "in the wild". |
@@ -1,5 +1,5 @@ | |||
{ | |||
"data_share_path": "2023Q4_20240424T120055Z", | |||
"data_share_path": "2023Q4_20240709T111731Z", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: data_share_path
is not a very intuitive name for the PACTA input directory
"pacta_data_quarter": "2023Q4", | ||
"project_code": "PA2024CH", | ||
"templates_ref": "", | ||
"peer_results": "https://pactadatadev.blob.core.windows.net/testing-files/peer-results/PA2024CH_peer-results-test", | ||
"peer_results": "https://pactadatadev.blob.core.windows.net/project-files/pa2024ch/peer_files", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's mentioned in the (very long) thread of this PR, but it's not super clear to me why/if the peer files should change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
So we are shifting to the real peer-results file (as opposed to the test files previously) since they had to be re-run to correctly contain the appropriate scenario data, as per RMI-PACTA/pacta.scenario.data.preparation#61
(This is also why the testing matrix has changed for CI/CD, since we are using the real files, the URL has been replaced with an az storage copy
command)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like me to document that in the PR body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't / don't know how to review the test results anymore, but otherwise seems reasonable
To test you can run this command: az storage copy --recursive --source https://pactadatadev.blob.core.windows.net/ghactions-workflow-transition-monitor-results-reports-private/rmi_pacta_2023q4_pa2024ch-20240717T142912Z/EN/bank/2/working_dir/50_Outputs/rmi_pacta_2023q4_pa2024ch --destination rmi_pacta_2023q4_pa2024ch-EN-bank However, since I believe you were never able to get the From portal.azure.com, navigate to the I know it's not the most intuitive but definitely possible if you have the gumption 😉 |
The newly created data directory now contains APS and STEPS scenarios for WEO 2023 for the aviation, cement and steel sectors.
Importantly also, we had to update paths to the new (real)
peer
files, as those had to be re-run with those same latest scenarios.With these scenarios missing, the executive summary couldn't render correctly.
Relates to:
RMI-PACTA/workflow.data.preparation/pull/238
RMI-PACTA/workflow.data.preparation/pull/237
RMI-PACTA/workflow.data.preparation/pull/236
RMI-PACTA/pacta.scenario.data.preparation/pull/61
RMI-PACTA/pacta.scenario.data.preparation/pull/63