[c++] fix Booster.refit() segfault with linear_tree on single-class data#7233
[c++] fix Booster.refit() segfault with linear_tree on single-class data#7233FranciscoRMendes wants to merge 4 commits into
Conversation
FranciscoRMendes
requested review from
StrikerRUS,
borchero,
guolinke,
jameslamb,
jmoralez and
shiyu1994
as code owners
FranciscoRMendes
changed the title
oops my bad. I thought this would be an easy fix for me, turns out I was wrong. |
|
removed changes for the other unrelated bug |
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Just one quick comment, will review more thoroughly when I can.
| if weight is not None: | ||
| if _is_pyarrow_array(weight): | ||
| if pa_compute.all(pa_compute.equal(weight, 1)).as_py(): | ||
| if pa_compute.all(pa_compute.equal(weight, 1)).as_py(): # type: ignore[attr-defined] |
There was a problem hiding this comment.
| if pa_compute.all(pa_compute.equal(weight, 1)).as_py(): # type: ignore[attr-defined] |
Please remove this and update this branch, this was fixed in #7236
And in the future, when you find yourself making changes that seem unrelated to the purpose of the PR, please consider proactively writing a small comment explaining them. Including the relevant warnings / error messages in plaintext is also helpful, because it allows others to find similar issues from GitHub search.
For this case, I did that here: #7238
Doing that before maintainers have to ask "wait, what is this about?" cuts out a review cycle, which helps us all work a bit more efficiently.
…g with linear trees (lightgbm-org#6792) Two bugs are fixed here: 1. Segfault in CalculateLinear when refitting a linear_tree model on data whose label contains only one class. Features that are constant in the refit dataset are filtered out during Dataset construction, so InnerFeatureIndex() returns -1 for them. The code then passed -1 directly to FeatureBinMapper(), causing an out-of-bounds crash. Fix: skip features whose InnerFeatureIndex() returns -1, and track their original position in the leaf's feature list so that the old coefficient used for the decay blend is taken from the correct slot. 2. Spurious "categorical_feature keyword found in params" UserWarning emitted by Dataset construction inside Booster.refit() when the model was loaded from a text file. The model text file embeds categorical_feature in the stored parameter string, which ends up in Booster.params and was forwarded verbatim as the params= argument to Dataset. Dataset.__init__ already accepts categorical_feature as an explicit argument, so having it in params triggers the duplicate-keyword warning. Fix: strip all categorical_feature aliases from the params dict that is passed to Dataset inside refit(), since categorical_feature is already forwarded as the dedicated Dataset argument.
…pe: ignore for pyarrow.compute stubs
FranciscoRMendes
force-pushed
the
fix/issue-6792-clean
branch
from
e6b2ecf to
dafbf25
Compare
|
I did three things,
Without (2) and (3), the guard in (1) would fix the crash but silently use the wrong old coefficients during refit. |
Conflict resolution accidentally removed the _choose_param_value block for categorical_feature that upstream gained via lightgbm-org#7124. Restore it so basic.py matches upstream exactly.
FranciscoRMendes
force-pushed
the
fix/issue-6792-clean
branch
from
dafbf25 to
9e1e5be
Compare
This comment was marked as spam.
This comment was marked as spam.
3 participants
Fixes the segfault in Booster.refit() reported in #6792.
When refitting a linear_tree=True model on data whose label has a single class, some features end up constant in that subset. LightGBM filters out constant features during Dataset construction, so Dataset.InnerFeatureIndex() returns -1 for them.
CalculateLinear was then calling FeatureBinMapper(-1) — an invalid index — which caused the segfault.
The fix skips features where InnerFeatureIndex() returns -1 in CalculateLinear. I added a leaf_old_coeff_idx vector to track each surviving feature's position in the stored coefficient array, so the decay blend (decay_rate * old_coeff + ...) still reads the
right slot when features are skipped.
I have tested this by running test_refit_linear_tree_single_class_no_segfault which confirms refitting on single-class data no longer crashes.
Closes #6792