You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[BUG] Same type(x) is tuple anti-pattern as #4372 in gradient/deep explainers and partial_dependence plot #4480
#4372 fixed data is dict in shap/maskers/_tabular.py and shap/explainers/_explainer.py (PRs #4373 / #4424). The same identity-check-against-type anti-pattern exists in four other files that were not covered by those PRs. type(x) is Y compares the exact concrete type and silently rejects subclasses — most notably, NamedTuple instances from PyTorch forward hooks (which are a common input to the gradient/deep explainers).
Locations
14 occurrences across 4 files, verified against current master:
shap/explainers/_gradient.py (4)
L75 — if type(model) is tuple:
L520 — if type(interim_inputs) is tuple:
L667 — if type(interim_inputs) is tuple:
L668 — if type(interim_inputs) is tuple: ← duplicate of L667; inner branch is either unreachable dead code or masking a typo
shap/explainers/_deep/__init__.py (1)
L72 — if type(model) is tuple:
shap/explainers/_deep/deep_pytorch.py (4)
L41 — if type(interim_inputs) is tuple:
L282 — if i != 0 and type(output) is tuple:
L288 — if type(input) is tuple:
L292 — if type(output) is tuple:
shap/plots/_partial_dependence.py (5)
L71 — if type(ind) is not tuple:
L216 — xmin0 = xmin[0] if type(xmin) is tuple else xmin
L217 — xmin1 = xmin[1] if type(xmin) is tuple else xmin
L218 — xmax0 = xmax[0] if type(xmax) is tuple else xmax
L219 — xmax1 = xmax[1] if type(xmax) is tuple else xmax
Why it matters
User-visible breakage: PyTorch forward hooks can return torch.return_types.* objects, which are tuple subclasses rather than tuple instances — these fail type(x) is tuple. Same for namedtuple/NamedTuple used by third-party models. The gradient and deep explainers silently take the wrong branch.
Latent bug at _gradient.py:667-668: Two nested if type(interim_inputs) is tuple: checks. Either the inner block is dead code, or it was meant to check something else (e.g. len(interim_inputs) > 0) and was accidentally duplicated. Worth inspecting before blindly replacing.
Replace all 14 occurrences with isinstance(x, tuple) / not isinstance(x, tuple). For the duplicated pair at _gradient.py:667-668, investigate git history to determine the original intent and fix accordingly, do not blindly delete the inner block.
I'd like to take this on as a single scoped PR (all four files, one concern: replacing the anti-pattern). Happy to split into one PR per file if that's easier to review.
AI disclosure
Investigation and drafting assisted by Claude. All locations and code citations verified manually against master.
Starting local work on this pending maintainer approval on the approach. Will open the PR once confirmed. Happy to adjust scope or split into per-file PRs.
Hey @SarthakB11 , I've submitted a PR for this: master...neha222222:shap:fix-type-is-tuple-antipattern — replaced all type(x) is tuple with isinstance(x, tuple) and cleaned up a duplicate dead-code check in _gradient.py.
Let me know if this solution looks good.
L667-668 duplicate if collapse — looks correctly resolved in your diff (the else branch is now reachable). Worth calling this out explicitly in the PR description since it's a behavior change (previously unreachable dead code is now live), not just a style fix.
Tests — the checklist shows "Unit tests added" unchecked. Since the L667-668 fix changes reachable code paths, a test exercising the non-tuple interim_inputs fallback (even a basic smoke test) would strengthen the PR.
Activity
SarthakB11 commentedon Apr 11, 2026
Starting local work on this pending maintainer approval on the approach. Will open the PR once confirmed. Happy to adjust scope or split into per-file PRs.
fix: use isinstance() instead of type() is tuple checks
neha222222 commentedon Apr 12, 2026
Hey @SarthakB11 , I've submitted a PR for this: master...neha222222:shap:fix-type-is-tuple-antipattern — replaced all type(x) is tuple with isinstance(x, tuple) and cleaned up a duplicate dead-code check in _gradient.py.
Let me know if this solution looks good.
SarthakB11 commentedon Apr 12, 2026
Thanks @neha222222 for picking this up — I see you've opened PR #4508. I had a local branch ready but happy to defer.
A few notes on review for #4508:
L667-668 duplicate
ifcollapse — looks correctly resolved in your diff (theelsebranch is now reachable). Worth calling this out explicitly in the PR description since it's a behavior change (previously unreachable dead code is now live), not just a style fix.AI disclosure — the SHAP CONTRIBUTING guidelines require explicit AI usage disclosure. If any AI tools were used, please add a disclosure section to the PR description. (Tobias closed PR refactor: extract TreeEnsemble handling into strategy pattern (#4212) #4314 specifically for missing this.)
Tests — the checklist shows "Unit tests added" unchecked. Since the L667-668 fix changes reachable code paths, a test exercising the non-tuple
interim_inputsfallback (even a basic smoke test) would strengthen the PR.fix: use isinstance() instead of type() is tuple checks (shap#4508)