Skip to content

Resolve last two open issues: #199 (mypy) and #342 (volumetric)#381

Merged
zihuaihuai merged 1 commit intomasterfrom
fix/issues-199-342
May 4, 2026
Merged

Resolve last two open issues: #199 (mypy) and #342 (volumetric)#381
zihuaihuai merged 1 commit intomasterfrom
fix/issues-199-342

Conversation

@zihuaihuai
Copy link
Copy Markdown
Collaborator

Summary

Closes the last two open issues that weren't resolved by #378 / #379 / #380.

Closes #199 — Remove # type: ignore from SLM class

SLM.py and _multiple_comparisons.py had file-level # type: ignore shebangs because mypy crashed on the in-class from ._linear_model import _linear_model pattern (the original blocker, python/mypy#10521). Move those imports to module level and bind the functions as class attributes (_linear_model = _linear_model, etc.). Mypy now runs to completion on both files instead of crashing.

The pre-existing untyped-code errors that the file-level ignore was hiding (~380 of them, mostly around Optional access without narrowing) are now surfaced but left to a follow-up PR — fixing them is genuinely a separate piece of work and re-enabling the commented-out mypy job in CI before that triage would just produce noise.

Closes #342 — Volumetric mixed effects

The reports on this issue all converged on the same two contract bugs in volumetric workflows:

  1. contrast dtype: pandas Series of object/categorical dtype was preserved through np.array(contrast). Downstream np.dot then raised TypeError: can't multiply sequence by non-int of type 'float', or (worse) the run silently produced all-NaN t-values. Now coerced to numeric in __init__, with a clear TypeError if conversion fails.
  2. mask dtype: integer 0/1 masks (the natural shape of nib.load(...).get_fdata().astype(int)) caused _fdr to do fancy indexing instead of boolean masking, producing the well-known shape (126006,) could not be broadcast to indexing result of shape (1082035,) error. Now coerced to bool in __init__.

Also reworded the surf=None + correction='rft' error so it explicitly points users at correction='fdr' for volumetric data — the previous "RFT corrections require a surface" message read as a dead-end.

The hard part of #342 — implementing cluster-level RFT/TFCE for volumes — is genuinely out of scope; SurfStat had it but BrainStat's port doesn't, and a faithful re-port would be a multi-week project. That's flagged in the new error message and remains tracked. What this PR does fix: every SLM(...) invocation pattern that the issue reporters posted now runs end-to-end.

Test plan

Four new tests in brainstat/tests/test_SLM.py:

  • test_volumetric_contrast_coercion — pandas Series → float; object dtype rejected with helpful message.
  • test_volumetric_int_mask_is_coerced_to_bool — int 0/1 mask becomes a boolean mask.
  • test_volumetric_rft_without_surface_explains_alternatives — error message mentions FDR.
  • test_volumetric_fdr_with_int_mask_runs_end_to_end — the full reporter scenario (int mask + pandas contrast + FDR) returns finite t-values inside the mask and a Q-array of the right shape.

Locally: 73/73 SLM + t_test + FDR + f_test cases pass.

Out of scope (kept open as separate trackers)

  • The 383 pre-existing typing errors that the file-level # type: ignore was hiding — straightforward but noisy cleanup.
  • Volumetric cluster-level RFT/TFCE (the ambitious half of Volumetric mixed effects #342).
  • Re-enabling the commented-out mypy job in python_unittests.yml once the typing errors are triaged.

#199 — Remove # type: ignore from SLM module

The `# type: ignore` shebangs at the top of `SLM.py` and
`_multiple_comparisons.py` were there because mypy crashed on the
in-class `from ._linear_model import _linear_model` style (see
python/mypy#10521). Lift those imports to module level and assign the
functions as class attributes; mypy now runs to completion on both
files. Pre-existing untyped-code errors are surfaced but unrelated to
the original blocker — re-enabling the commented-out mypy job in CI is
a separate follow-up once those are triaged.

#342 — Volumetric mixed effects: contract & error-message fixes

The earlier reports on this issue were all hitting the same two
foot-guns when feeding volumes to SLM:

1. `contrast` came in as a pandas Series of object/categorical dtype
   (or similar). `np.array(contrast)` preserved that dtype, so `np.dot`
   later raised the cryptic `TypeError: can't multiply sequence by
   non-int of type 'float'` — and (depending on path) silently produced
   all-NaN t-values.
2. `mask` came in as a 0/1 *integer* array (the natural shape of
   `nib.load(...).get_fdata().astype(int)`). The downstream
   `Q[self.mask] = ...` path inside `_fdr` then degenerated into fancy
   indexing and threw a shape-mismatch (`shape (126006,) could not be
   broadcast to indexing result of shape (1082035,)`).

Add `_coerce_contrast` / `_coerce_mask` helpers in `__init__` so both
inputs are normalised once at construction time. Categorical/object
contrasts now raise a clear `TypeError` pointing at the fix. Integer
masks are converted to bool transparently.

Also reword the `surf=None + correction='rft'` error to point users at
`correction='fdr'` for volumetric data, so the failure mode the issue
describes ("RFT corrections require a surface" → user is stuck) is no
longer a dead-end.

Test plan: 4 new tests in `test_SLM.py` cover the contrast coercion,
mask coercion, the new RFT error message, and the full int-mask + FDR +
pandas-Series-contrast path that several reporters were originally
hitting. Existing `test_volumetric_input` and the rest of the SLM /
t-test / FDR / F-test suites still pass (73/73 locally).
@github-actions github-actions Bot added the Python On the Python implementation label May 4, 2026
@zihuaihuai zihuaihuai merged commit c0aedb4 into master May 4, 2026
7 checks passed
@zihuaihuai zihuaihuai deleted the fix/issues-199-342 branch May 4, 2026 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Python On the Python implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Volumetric mixed effects [REF] Remove #type: ignore from SLM class

1 participant