-
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
simplify and speed up comparisons for splits with identical gains #4542
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.
I believe now these if (local_gain != other_gain)
statements can be moved upper (right before int local_feature = this->feature;
line) for the speedup.
oooo yeah, good idea! I'll do that here |
updated in b40452e great suggestion @StrikerRUS , this is going to make the majority of split comparisons a little bit faster, very nice 😎 |
oops, forgot |
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 fine to me, thank you!
But I don't want to be the only reviewer for this PR due to my unfamiliarity with cpp.
Thanks, totally understand! @shiyu1994 or @btrotta could you please provide a review here (and on #4496 )? |
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.
A good rewrite to make the code much cleaner. But I think the original implementation is also correct. In the old implementation of operator==
, it is saying, when local_gain != other_gain
, the result of ==
is false
. Since when local_gain != other_gain
is true
. local_gain==other_gain
is false.
But I still think this modification is quite valuable.
ahhh ok yes @shiyu1994 you're right! I got confused by this if (local_gain != other_gain) {
return local_gain == other_gain;
} I'll change the title of the PR before we merge, to make it clear that this is a cleanup / small optimization and that it isn't actually fixing anything "incorrect". |
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. |
Short Summary
This PR fixes a bug in the
==
operator forSplitInfo
andLightSplitInfo
insrc/treelearner/split_info.hpp
. In the event that two candidate splits have different gains,==
currently incorrectly says that those splits are identical.I'm not certain, but I believe this is a bug that could lead to incorrect behavior in voting parallel learning, at the stage where learners select the top
k
features to consider for future splits. I'm not sure if this fix would fix #4414, but if I'm right then it might at least help.Longer Description
The
SplitInfo
class is used to store information about a candidate tree split, including the gain of the split and the index of the feature it refers to. There is also a lighter-weight alternative to this class, calledLightSplitInfo
.LightGBM's training process involves calculating the gain for different splits and comparing those splits to each other to determine the best one. To do this, it defines
>
and==
operators for comparing twoSplitInfo
orLightSplitInfo
instances.Running
cppcheck
(#4539), I found what looks like a bug in the definition of the==
operator forSplitInfo
andLightSplitInfo
!This code (introduced in 4f77bd2), has what looks like a mistake:
That code is saying "if the gain of this split and the split being compared to it are different,
this_split == that_split
should betrue
".How this fix affects
LightGBM
I tried commenting out the definitions of those
==
operator insrc/treelearner/split_info.hpp
to see where it's used. It's used much less frequently than the<
operator, but I found at least one place...in the stage where voting parallel training evaluates splits to pick the topk
features to consider!LightGBM/src/treelearner/voting_parallel_tree_learner.cpp
Line 173 in a926d4f
LightGBM/include/LightGBM/utils/array_args.h
Line 160 in a926d4f
LightGBM/include/LightGBM/utils/array_args.h
Line 137 in a926d4f
LightGBM/include/LightGBM/utils/array_args.h
Lines 118 to 119 in a926d4f
A reminder from the original LightGBM paper of how this affects training: