reproducible parameter alias resolution for wrappers (fixes #5304) #5338
Conversation
jmoralez
requested review from
StrikerRUS,
guolinke,
shiyu1994 and
jameslamb
as code owners
There was a problem hiding this comment.
Very very nice work! Please see a few initial suggestions. I'll take a look again soon, but overall I'm supportive of the approach you took and I'm confident that these code paths are heavily tested.
I'd also like to wait to merge this until @StrikerRUS has a chance to review as well.
| @@ -55,7 +55,7 @@ test_that(".PARAMETER_ALIASES() returns a named list of character vectors, where | |||
| expect_true(length(names(param_aliases)) == length(param_aliases)) | |||
| expect_true(all(sapply(param_aliases, is.character))) | |||
| expect_true(length(unique(names(param_aliases))) == length(param_aliases)) | |||
| expect_equal(sort(param_aliases[["task"]]), c("task", "task_type")) | |||
There was a problem hiding this comment.
Sorry, looking at this again....why does this assertion need to be removed?
I understand why this PR adds the other one below that doesn't use sort(), but could we still keep the rest of this test intact? That would give me a bit more confidence that this change is working as expected, and having tests here referencing two different parameters increases the likelihood of catching bugs of the form ".PARAMETER_ALIASES() returns unexpected output for some but not all parameters".
There was a problem hiding this comment.
Restored in e45fc48
|
@shiyu1994 I'd appreciate your review if you have time. |
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Extracts the current logic on the C++ side to sort aliases to a function
Config::SortAliasso thatConfig::DumpAliasescan use the same criteria to sort them and allow the wrappers to be in sync.Fixes #5304