Allow sector borderline input (#326) #327
Conversation
Here I check if the input loanbook has sector or borderline before joining the original columns back.
jdhoffa
changed the title
There was a problem hiding this comment.
Do you remember why we chose to not allow using "sector" before? Maybe to avoid complicating the code with assertions to ensure sector was valid?
If there is no good reason, then this LGTM and I approve.
If there is a good reason to not allow using "sector", then it might be safer to start the body of abort_reserved_column() with something like this:
if (getOptions("r2dii.match_abort_reserved_column") == "dissable") {
return(invisible(data))
}Then in r2dii.analysis we could set options(r2dii.match_abort_reserved_column = "dissable") in our testing environment so that match_name() let us use reserved columns.
|
After running a It looks as though, through some refactoring, we may have accidentally fixed this bug, and now it seems as though providing a I'm happy with this solution, and would like to merge as is, but will wait for you to have a look at this and see if you agree. |
Closes #326 Closes #327 -- this is a simpler/safer way to solve it. Relates to: * https://github.com/2DegreesInvesting/r2dii.analysis/pull/227 * https://github.com/2DegreesInvesting/r2dii.analysis/pull/230 This change is internal, and motivated by our need to break the dependency between r2dii.data and its reverse dependencies. The problem we are trying to solve is this: r2dii.data provides data that is used in some functions if its reverse dependencies -- like r2dii.match. Concretely, changes in the sector classification dataset (in r2dii.data) make tests to fail in reverse dependencies like r2dii.match that store a snapshot of expected output (e.g. of match_name()). This commit allows us to use generate such snaphsots with a frozen loanbook, and thus make r2dii packages less prone to meaningless errors when CRAN checks for reverse dependencies.
|
Closing this in favor of #328 |
match_namenow accepts aloanbookobject that may or may not have thesectorandborderlinedefined. If defined, it will use these values, otherwise, it will join our proprietary sector classifications.A fun side-product of this is that users can now manually set the sectors!
Relates to 2DegreesInvesting/r2dii.analysis#227
Closes #326