2DegreesInvesting / r2dii.climate.stress.test

Functions and workflows to run financial stress tests on climate change related risk types
https://2degreesinvesting.github.io/r2dii.climate.stress.test
Other
20 stars 12 forks source link

is `{ggplot2}` actually necessary for web tool? #278

Closed cjyetman closed 2 years ago

cjyetman commented 2 years ago

{ggplot2} is loaded in the web tool scripts, but it doesn't appear to be used anywhere (though it's possible it gets used somehow indirectly). It would be advantageous if we could remove these library(ggplot2) lines and not have to depend on {ggplot2} for the transitionmonitor Docker. Not failing on these two lines is currently the only reason that I am aware of that we need to include it in the Docker image for. https://github.com/2DegreesInvesting/r2dii.climate.stress.test/blob/b326c65fce7e0e09cfd70a63f67aa957477e7503/web_tool_stress_test.R#L4 https://github.com/2DegreesInvesting/r2dii.climate.stress.test/blob/b326c65fce7e0e09cfd70a63f67aa957477e7503/web_tool_external_stress_test.R#L7

cjyetman commented 2 years ago

After further investigation, {ggplot2} is needed for the create_interactive_report under certain conditions, even if it is not explicitly loaded anywhere. However, I would still suggest removing these library(ggplot2) calls here unless they are truly needed by the stress testing web tools.

ghost commented 2 years ago

Just so I understand @cjyetman , you say that {ggplot2} is needed for the create_interactive_report, how can this be done if ggplot 2 is not included in the transitionmonitor Docker?

Otherwise I am all for getting rid of ggplot as a dependency of the stresstesting package, i.e. removing the library() call in the web tolls scripts and also moving ggplot from imports to suggest in DESCRIPTION. However as this is a webtool thing i think @jacobvjk should also approve the suggestion, in case the library call were originally added for a reason.

cjyetman commented 2 years ago

{ggplot2} will be installed in the transitionmonitor Docker image because it is required for code in the create_interactive_report repo that runs if an executive summary is being created.

However, loading {ggplot2} here in the stress test web tool code with library(ggplot2) is completely unnecessary, as far as I understand

ghost commented 2 years ago

Ok, will push a PR and see what the integration test and jacob think about it :)