fix(tree): LinearTreeSHAP no longer mutates the caller-owned TreeModel#546
Open
42logos wants to merge 1 commit into
Open
fix(tree): LinearTreeSHAP no longer mutates the caller-owned TreeModel#54642logos wants to merge 1 commit into
42logos wants to merge 1 commit into
Conversation
validate_tree_model returns TreeModel inputs by reference, and LinearTreeSHAP immediately called reduce_feature_complexity() on the result — mutating, in place, a TreeModel the caller may share with other explainers (their feature ids get remapped under them, silently corrupting subsequent computations). Deepcopy the validated model before reducing. Minimal reproduction: construct a TreeModel over data with more features than the tree uses, snapshot TreeModel.features, construct LinearTreeSHAP(tree_model) — the snapshot differs without this fix.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a correctness bug where constructing LinearTreeSHAP could mutate a caller-owned TreeModel in place (via reduce_feature_complexity()), corrupting feature-id metadata for downstream reuse/inspection. The fix aligns LinearTreeSHAP with the existing TreeSHAPIQ behavior by deep-copying the validated TreeModel before any in-place reduction.
Changes:
- Deep-copy the validated tree in
LinearTreeSHAP.__init__before callingreduce_feature_complexity(). - Add a regression test ensuring
LinearTreeSHAPconstruction does not mutate the inputTreeModeland still produces a finite explanation. - Document the bugfix in
CHANGELOG.mdunder an Unreleased section.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/shapiq/tree/linear/explainer.py |
Prevents in-place mutation of caller-owned TreeModel by deep-copying prior to feature reduction. |
tests/shapiq/tests_unit/tests_explainer/tests_tree_explainer/test_tree_bugfix.py |
Adds regression coverage verifying the input TreeModel is unchanged and the explainer still functions. |
CHANGELOG.md |
Notes the behavioral fix and links the related issue. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #544.
What was wrong
Constructing a
LinearTreeSHAPexplainer silently mutated theTreeModelpassed to it:validate_tree_modelreturnsTreeModelinputs by reference, and the constructor then calledreduce_feature_complexity()on that result, rewriting itsfeatures/feature_ids/n_features_in_treein place. A user inspecting the model afterwards — or reusing it with another explainer — read corrupted feature ids (e.g. the real ids[5, 7]overwritten with internal[0, 1]). See #544 for an end-to-end sklearn reproduction.TreeSHAPIQalready deep-copies before its ownreduce_feature_complexity();LinearTreeSHAPwas the inconsistent one.Fix
Deepcopy the validated tree before reducing — one line in
src/shapiq/tree/linear/explainer.py, matchingTreeSHAPIQ. The deepcopy is a one-time construction cost on a singleTreeModel(a handful of NumPy arrays), negligible next to the N-matrix setup that follows.Testing
test_linear_tree_shap_does_not_mutate_caller_tree_modelintest_tree_bugfix.py: snapshotsTreeModel.features, constructsLinearTreeSHAP, asserts the caller's object is untouched and the explainer still works on its private reduced copy. Verified red without the fix, green with it.tests_tree_explainersuite passes;ruff check/ruff formatclean on the changed files (zero new findings vsmain).