search

RMI-PACTA / r2dii.match

Tools to Match Financial Portfolios with Climate Data
https://rmi-pacta.github.io/r2dii.match
Other
7 stars 7 forks source link

`r2dii.match.sector_classifications` option is an awkward interface #498

Open cjyetman opened 3 weeks ago

cjyetman commented 3 weeks ago

Setting the r2dii.match.sector_classifications option to use a custom sector classification seems like an awkward interface. It also appears that it was added as an experimental/temporary thing but never got upgraded to a normal/standard argument (see https://github.com/RMI-PACTA/r2dii.match/pull/354#discussion_r590588231). It's currently in active use in RMI-PACTA/workflow.multi.loanbook e.g. https://github.com/RMI-PACTA/workflow.multi.loanbook/blob/e6de82182b36c1c0504494bc6b2cf7a505d7d19d/R/run_matching.R#L131-L147

maybe it's time to reconsider making r2dii.match.sector_classifications a proper argument?

jdhoffa commented 3 weeks ago

You are 100% correct, the interface was never really meant to make it to production, it was mostly an experimental and low-effort way to use custom classifications IF you had to.

I agree that the interface is awkward and we can do better.

I think gaining the argument, with a default value to ensure backwards compatibility is a great idea.

@jacobvjk @cjyetman do you think this is a priority, so we can use it in workflow.multi.loanbook? If so, I think we can chuck it into the next sprint, and do a relatively low-lift CRAN release in the next few weeks.