-
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
Fix number of features mismatching in continual training (fix #5156) #5157
base: master
Are you sure you want to change the base?
Changes from 10 commits
24b7df0
442c03a
2887ac0
0a20a13
bda5bdb
7e7190f
adb8982
c823137
750d35c
3e52538
23e9991
d719d78
aab8d1f
1f09936
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -970,6 +970,29 @@ def test_continue_train_multiclass(): | |||||||||||||||||||||||||||
assert evals_result['valid_0']['multi_logloss'][-1] == pytest.approx(ret) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
def test_continue_train_different_feature_size(capsys): | ||||||||||||||||||||||||||||
np.random.seed(0) | ||||||||||||||||||||||||||||
train_X = np.random.randn(100, 10) | ||||||||||||||||||||||||||||
train_y = np.sum(train_X, axis=1) | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, strange. With
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now a warning is provided, see https://github.com/microsoft/LightGBM/pull/5157/files#r868752144. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Isn't true for me 😕 Output:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry about that and thanks for checking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you very much for working on the test! Log:
Model:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @StrikerRUS I've tested with v3.3.2 with the test case def test_continue_train_different_feature_size(capsys):
np.random.seed(0)
train_X = np.hstack([np.ones(800).reshape(-1, 8), np.arange(200, 0, -1).reshape(-1, 2)])
train_y = np.sum(train_X[:, -2:], axis=1)
train_data = lgb.Dataset(train_X, label=train_y)
params = {
"objective": "regression",
"num_trees": 10,
"num_leaves": 31,
"verbose": -1,
'predict_disable_shape_check': True,
}
model = lgb.train(train_set=train_data, params=params)
train_X_cont = np.random.rand(100, 5)
train_y_cont = np.sum(train_X_cont, axis=1)
train_data_cont = lgb.Dataset(train_X_cont, label=train_y_cont)
params.update({"verbose": 2})
lgb.train(train_set=train_data_cont, params=params, init_model=model)
captured = capsys.readouterr()
assert captured.out.find("features found in dataset for continual training, but at least") != -1 And the output is
which shows that the test case fails in v3.3.2. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shiyu1994 This test should segfault or something like this (I don't remember exactly) in Warning was added after starting this review thread and we don't speak about it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @StrikerRUS Thanks. With the latest commit, it should now fail in assert captured.out.find("features found in dataset for continual training, but at least") != -1 in the latest version of test case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Still doesn't fail in Please use fixed dataset, not a random one, to constantly reproduce segfaults in versions without workaround from this PR. |
||||||||||||||||||||||||||||
train_data = lgb.Dataset(train_X, label=train_y) | ||||||||||||||||||||||||||||
params = { | ||||||||||||||||||||||||||||
"objective": "regression", | ||||||||||||||||||||||||||||
"num_trees": 10, | ||||||||||||||||||||||||||||
"num_leaves": 31, | ||||||||||||||||||||||||||||
"verbose": -1, | ||||||||||||||||||||||||||||
'predict_disable_shape_check': True, | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it necessary to pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good observation! But LightGBM/python-package/lightgbm/engine.py Lines 153 to 154 in 0018206
LightGBM/python-package/lightgbm/engine.py Lines 160 to 161 in 0018206
LightGBM/python-package/lightgbm/basic.py Lines 1519 to 1522 in 0018206
LightGBM/python-package/lightgbm/basic.py Line 1393 in 0018206
LightGBM/python-package/lightgbm/basic.py Lines 1399 to 1402 in 0018206
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see! So then is the bug addressed by this PR only present when you've also set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, without setting
I think the second sentence of the error "You can set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
model = lgb.train(train_set=train_data, params=params) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
train_X_cont = np.random.rand(100, 5) | ||||||||||||||||||||||||||||
train_y_cont = np.sum(train_X_cont, axis=1) | ||||||||||||||||||||||||||||
train_data_cont = lgb.Dataset(train_X_cont, label=train_y_cont) | ||||||||||||||||||||||||||||
params.update({"verbose": 2}) | ||||||||||||||||||||||||||||
lgb.train(train_set=train_data_cont, params=params, init_model=model) | ||||||||||||||||||||||||||||
captured = capsys.readouterr() | ||||||||||||||||||||||||||||
assert captured.out.find("features found in continually trained model, but at least") != -1 | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
def test_cv(): | ||||||||||||||||||||||||||||
X_train, y_train = make_synthetic_regression() | ||||||||||||||||||||||||||||
params = {'verbose': -1} | ||||||||||||||||||||||||||||
|
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.
Provide warning instead of enlarging feature importance vector. Since
GBDT::FeatureImportance
is used by Python API and the python code doesn't know the final size of feature importance vector, dynamically enlarging the vector will cause out of bound access.