RMI-PACTA / pacta.portfolio.report

pacta.portfolio.report
https://rmi-pacta.github.io/pacta.portfolio.report/
MIT License
1 stars 0 forks source link

prep_emissions_trajectory calls `start_year` from global environment #43

Closed AlexAxthelm closed 7 months ago

AlexAxthelm commented 7 months ago

https://github.com/RMI-PACTA/pacta.portfolio.report/blob/5162593ee62be47ff46c493b93ecd48ff2203b8d/R/prep_emissions_trajectory.R#L47

This function calls .env$start_year in a dplyr chain, without it being supplied as an argument to the function.

cjyetman commented 7 months ago

nice catch! I'm fairly certain that was unintentionally missed in https://github.com/RMI-PACTA/pacta.interactive.report/pull/132 (done in the predecessor of this repo)

AlexAxthelm commented 7 months ago

Am I missing something about inheritence? start_year is defined in the calling environment, but doesn't show up in the function's env.

Browse[2]> parent.frame()$start_year
[1] 2022
Browse[2]> start_year
Error in prep_emissions_trajectory(equity_results_portfolio, bonds_results_portfolio,  :
  object 'start_year' not found
cjyetman commented 7 months ago

When I extracted the code like that into functions, the intent was to make every value necessary a function argument so that it could be input into the function when called in create_interactive_report(). I missed start_year in this case... missed as in I did not add it as a function argument... so I unintentionally made prep_emissions_trajectory() reliant on a value in the calling environment, whereas the intent was to allow passing in start_year as an argument and rely on a local (to the function) copy of start_year.

cjyetman commented 7 months ago

so this https://github.com/RMI-PACTA/pacta.portfolio.report/blob/5162593ee62be47ff46c493b93ecd48ff2203b8d/R/prep_emissions_trajectory.R#L1-L9

should be changed to

prep_emissions_trajectory <- 
   function(equity_results_portfolio, 
            bonds_results_portfolio, 
            investor_name, 
            portfolio_name, 
            select_scenario_other, 
            select_scenario, 
            twodi_sectors, 
            year_span,
            start_year) { 

and this https://github.com/RMI-PACTA/pacta.portfolio.report/blob/5162593ee62be47ff46c493b93ecd48ff2203b8d/R/create_interactive_report.R#L337-L347

should be changed to

data_emissions <- 
   prep_emissions_trajectory( 
     equity_results_portfolio, 
     bonds_results_portfolio, 
     investor_name, 
     portfolio_name, 
     select_scenario_other, 
     select_scenario, 
     twodi_sectors, 
     year_span,
     start_year
   )