Skip to content

[BUG] Same type(x) is tuple anti-pattern as #4372 in gradient/deep explainers and partial_dependence plot #4480

Closed

Description

@SarthakB11

Context

#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

  1. 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.
  2. 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.
  3. Consistency with BUG: Type check uses is instead of isinstance in Impute class #4372 direction: The fix there used isinstance(data, dict). Applying the same pattern here keeps the codebase consistent.

Proposed fix

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.

Activity

  1. SarthakB11 commented on Apr 11, 2026

    @SarthakB11
    ContributorAuthor

    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.

  2. added a commit that references this issue on Apr 12, 2026
    bc65208
  3. neha222222 commented on Apr 12, 2026

    @neha222222
    Contributor

    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.

  4. SarthakB11 commented on Apr 12, 2026

    @SarthakB11
    ContributorAuthor

    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:

    1. 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.

    2. 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.)

    3. 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.

  5. added a commit that references this issue on May 27, 2026
    8549e6b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions