Update scenario executive summary #315
Conversation
|
This PR is dependent on: RMI-PACTA/pacta.executive.summary#325 that is why I marked it as 'draft'. |
Docker build status
|
|
@MonikaFu RMI-PACTA/pacta.executive.summary#325 has been merged, but this is still in draft mode. Is it ready for review? |
|
It's probably best after we also merge this one: RMI-PACTA/pacta.executive.summary#327 (comment). Then we have the right datasets for calculating the scores. |
|
It's ready for review now. |
|
I'm seeing loads of "an error occured when generating this plot" in the CI/CD results, e.g.: |
|
I manually re-triggered Docker build and test off this branch here: Wondering if maybe it didn't pick up the latest changes in |
|
Agreed @jdhoffa. It seems that the docker build is not always picking up the changes made to other PRs. It happened before. |
FYI... usually if a PR is created (technically, if an action runs) before a change in another repo is available on its main branch, re-running the action will not pickup new changes on the other repo... it seems to cache whatever was there the first time the action was run. So, if that situation is relevant here, you may want to open a new PR with the same changes here and see if the action succeeds with that. |
More specificly, the behavior I've observed (but not seen documented anywhere) is For a given You can trigger a new "run" (with new caching of state) by pushing a new commit. For example, see this PR (RMI-PACTA/workflow.pacta#59), where I add and delete trivial whitespace in order to explicitly trigger new |
|
The issues seems to persist any case: 
|
|
This is very strange because when I run the PR locally with all the latest versions of packages on |
Are you using the same testing data? |
Now yes I believe and I still get some plots to appear although some do not. But I don't get the same pdf as @jdhoffa with no plots generated. |
|
First thing I discovered is that in the indices data the scenario name we want to be using from Please note that the indices files exist in two places, |
Do we know why we used different files in the first place? I'm generally in favor of integrating them though. |
For the record, I did know/ notice that indices files had to be copied into the |
|
I'm pretty sure @cjyetman does not have insights here. It is an artifact of ES code. I guess it is easier to access them this way from within the ES template but I'm not sure why the person who wrote this code did not access indices the same way they access pacta results. |
|
Are the indices files in the workflow.transition.monitor/web_tool_script_3.R Lines 422 to 423 in 26473e5 workflow.transition.monitor/web_tool_script_3.R Lines 360 to 361 in 26473e5 which are the ones prepared directly by workflow.transition.monitor/web_tool_script_3.R Lines 170 to 172 in 26473e5 @MonikaFu can you try removing the indices data from |
|
@jdhoffa I'm pretty sure I did in the past and it failed. |
|
Here what I get locally. |
|
Just ran it locally with the latest Azure data, and I also get the same "error no plot" issue. So evidently there is some difference between our local set-ups. But in any case, I can reproduce the issue that's happening on CI/CD, so will look into it further. |
web_tool_script_3.R
Outdated
| scenario_source = "GECO2023", | ||
| scenario_selected = "1.5C", | ||
| scenario_source = "WEO2023", | ||
| scenario_selected = "NZE", |
There was a problem hiding this comment.
| scenario_selected = "NZE", | |
| scenario_selected = "NZE_2050", |
The April 24th run of data prep has this specification for scenario name:
Also in line with what the config file expects:
There was a problem hiding this comment.
This is also how it is specified in pacta.portfolio.report:
https://github.com/RMI-PACTA/pacta.portfolio.report/blob/0c1837ea39e4f9acfffdacb53bd049c11048bb3c/inst/js/trajectory_alignment.js#L423-L428
There was a problem hiding this comment.
for future reference, the scenario-source pairs are included in the manifest
jsonlite::read_json("~/Downloads/2023Q4_20240527T092929Z/manifest.json")$parameters$output_stats$source_scenario_pairsThere was a problem hiding this comment.
or better...
manifest <- jsonlite::fromJSON("~/Downloads/2023Q4_20240527T092929Z/manifest.json")
manifest$parameters$output_stats$source_scenario_pairs
#> scenario_source scenario
#> 1 GECO2023 1.5C
#> 2 GECO2023 NDC-LTS
#> 3 GECO2023 Reference
#> 4 ISF2023 1.5°C
#> 5 WEO2023 APS
#> 6 WEO2023 NZE_2050
#> 7 WEO2023 STEPS
web_tool_script_3.R
Outdated
| scenario_source = "GECO2023", | ||
| scenario_selected = "1.5C", | ||
| scenario_source = "WEO2023", | ||
| scenario_selected = "NZE", |
There was a problem hiding this comment.
| scenario_selected = "NZE", | |
| scenario_selected = "NZE_2050", |
|
FYI @MonikaFu I think it still won't work until RMI-PACTA/pacta.executive.summary#330 is merged. From what I remember, parts of the executive summary (unfortunately) still depend on default parameters |
|
It depends on the |
|
Re-triggered CI/CD, we shall see what happens there. Indices data has already been updated on Azure |
FYI, I think @AlexAxthelm clarified that re-triggering (telling GitHub to re-run) will use cached versions of external repos (therefor, not pick up recent changes to external repos), but adding a new commit to this PR will clear the cache and re-run with most recent version of external repos. |
|
@cjyetman just added and removed a line of code, let's see if this works. |
|
seems like the plots are still erroring in the results, no? |
|
@cjyetman yes, indeed, it looks like it. Looking into it now and trying to add missing pieces with RMI-PACTA/pacta.executive.summary#331. |
|
Alright, so I looked into this PR more (locally) and it seems like after merging this fix: RMI-PACTA/pacta.executive.summary#331 most of the plots should show up in the executive summary (with the exception of the scores, 2nd page, bottom). I manually updated the indices locally to match the ones used for this run both in the folder with pacta data and in |
|
@MonikaFu do you have any idea why things run as expected on your machine but not here? To me, that's a major source of confusion making this very difficult to resolve. I think we should try to get things aligned before committing to various fixes that don't actually work. |
My hunch would be that @MonikaFu's local data was out of sync, since locally she was able to produce a report prior to the |
|
@MonikaFu what is the status of this? |
|
Merging this as both PR dependencies have merged and the output on main looks pretty good, and this PR just changes the preferred scenario to use as a comparison in the exec summary. If follow-on changes need to be made, we can do that in a future PR. |
Updating scenario source and scenario for executive summary calculations. Aligned with what the team decided to use.