-
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
Add Cost Effective Gradient Boosting #2014
Conversation
Like the original CEGB version, this inherits from SerialTreeLearner. Currently, it changes nothing from the original.
This is heavily based on the serial version, but just adds using the coupled penalties.
…rhead of CEGB, and add sanity checks for the lengths of the penalty vectors.
The tree learner did not update the gains of previously computed leaf splits when splitting a leaf elsewhere in the tree. This caused it to prefer new features due to incorrectly penalising splitting on previously used features.
is this ready to merge ? |
@StrikerRUS: It's ready from my perspective. |
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.
Some minor notes.
I think we should cite CEGB somehow... |
@@ -496,6 +530,14 @@ void SerialTreeLearner::FindBestSplitsFromHistograms(const std::vector<int8_t>& | |||
smaller_leaf_splits_->max_constraint(), | |||
&smaller_split); | |||
smaller_split.feature = real_fidx; | |||
smaller_split.gain -= config_->cegb_tradeoff * config_->cegb_penalty_split * smaller_leaf_splits_->num_data_in_leaf(); |
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.
config_->cegb_tradeoff * config_->cegb_penalty_split
is zero by default, right?
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.
Yes. By default, this line doesn't change the gain, and so doesn't change the behaviour.
It only makes a difference if the user specifies a cegb_penalty_split.
I think it will be better to add a section into advaced-topics: https://github.com/Microsoft/LightGBM/blob/master/docs/Advanced-Topics.rst, about how to use cfgb. |
I've added a section to the docs on using CEGB, including a link to the paper. |
Thanks @remcob-gr ,it looks good to me. |
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 @remcob-gr !
Also, it'll be great if you can add some tests.
@StrikerRUS : I've added some tests. Could you take a look and see if there are other tests you'd like? |
@remcob-gr Perfect! Many thanks! @guolinke Can we merge? |
@StrikerRUS sure |
Fixes #1119 .
The implementation is in the form of a tweak to the serial tree learner and so should work with every derived tree learner, though I've only tested in serial.