-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
reproducible parameter alias resolution for wrappers (fixes #5304) #5338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot for doing this!
Just one question for my better understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just left one minor suggestion. Feel free to merge this once that's addressed.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored in e45fc48
@shiyu1994 I'd appreciate your review if you have time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, LGTM!
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::SortAlias
so thatConfig::DumpAliases
can use the same criteria to sort them and allow the wrappers to be in sync.Fixes #5304