Skip to content
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

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

Open
cjyetman opened this issue Aug 18, 2024 · 1 comment
Open

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

cjyetman opened this issue Aug 18, 2024 · 1 comment

Comments

@cjyetman
Copy link
Member

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 #354 (comment)). 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
Copy link
Member

jdhoffa commented Aug 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants