Repository navigation
[c++] Fix SHAP computation after model.update() - #7501
ron-wettenstein wants to merge 2 commits into
Conversation
…all (fixes lightgbm-org#7488) Since lightgbm-org#6778, GBDT::InitPredict() computes the trees' max depth (needed by TreeSHAP) only on the first pred_contrib call of a Booster, guarded by models_initialized_. That flag was never reset, so trees added afterwards kept max_depth_ = -1 and the next pred_contrib call failed `CHECK_GE(max_depth_, 0)`: e.g. pred_contrib between Booster.update() calls, or inside a training callback. Reset models_initialized_ whenever trees are added: in GBDT::TrainOneIter() (also used by DART and GOSS), RF::TrainOneIter() and GBDT::MergeFrom(). Tests: * Python: pred_contrib after Booster.update() and inside a callback * C++: single-row pred_contrib after LGBM_BoosterMerge() * R: feature contributions sum to the raw score Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Will look at it today, thanks a lot! |
mayer79
left a comment
mayer79
left a comment
There was a problem hiding this comment.
Looks very good.
For me it is fine to add a simple SHAP addititivity and dimension test to the R tests in this PR as well, given that the issue solved in this PR is related to SHAP.
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Thanks, the explanation and fix make sense to me. Some small suggestions.
| } | ||
| }) | ||
|
|
||
| test_that("feature contributions sum to the raw score", { |
There was a problem hiding this comment.
| test_that("feature contributions sum to the raw score", { | |
| test_that("feature contributions sum to the raw score for binary classification", { |
There are already tests covering this:
LightGBM/R-package/tests/testthat/test_Predictor.R
Lines 329 to 337 in 064a6db
It's fine if you want to add this standalone test but please:
- expand it to cover the other things you see there (like checking that the column with the predicted value contains correct-looking values)
- clarify in the test name that you're only checking binary classification (you yourself just explained in pred_contrib value doesn't add up to prediction in lightgbm regression #3998 (comment) how this is not true for all loss functions)
| objective = "binary" | ||
| , num_threads = .LGB_MAX_THREADS | ||
| ) | ||
| , nrounds = 20L |
There was a problem hiding this comment.
| , nrounds = 20L | |
| , nrounds = 5L |
The correctness of this test doesn't depend on the model depth, let's do something even smaller and cheaper please.
|
@mayer79 when you review here in the future, you're welcome to choose the PR label you think makes the most sense. That informs how PRs are categorized in the release notes, these categories: LightGBM/.github/release-drafter.yml Lines 3 to 15 in 064a6db @ron-wettenstein you can ignore the |
Fixes #7488 .
The fix is simple, we need to mark the new trees for initialization by setting
models_initialized_ = false.I also find similar bug in the case of merging trees, so I also added
models_initialized_ = falsein the relevant function. The tree merging bug is not something that one can reach using the public R/python API, still we better fix this so it won't hurt us in the future.Basically all the code here are 3 lines of
models_initialized_ = false.The rest are tests:
bst$.__enclos_env__$private$handle. Then I notices no simple SHAP test exist there, so I added it. This test is not directly related to this PR, if you prefer I can remove it.