Fix ty type-check diagnostics repo-wide (496 -> 0)#124
Merged
Conversation
Fit-dependent attributes (W, A, c, mu, alpha, beta, rho, pdtype, gm, comp_list, mean, sphere) are None until fit() runs, then reassigned to real tensors; ty correctly flagged every downstream use as a possible None-subscript/attribute error. Explicit Optional[torch.Tensor] annotations plus assert narrowing at each internal hot-path call site (E-step, M-step, component sharing, adaptive PDF switch) resolved most of these; the four public accessors that a user could call before fit() (transform, get_mixing_matrix, get_unmixing_matrix, variance_order) get a real RuntimeError instead, matching the existing state_dict() convention. Also fixed a genuine possibly-unbound risk in _get_block_updates: self.do_newton was checked twice without a local binding, so ty couldn't prove the two checks agree; hoisted into a local. No behavior change (assertions only, all true at their call sites); full torch_tests suite green (130 passed, 5 skipped).
Same pattern as torch_impl/core.py: fit-dependent attributes are None until fit() runs. Explicit Optional[np.ndarray]/Optional[int] annotations plus assert narrowing at internal call sites; transform() and load_default_params() get real checks/type widening since they can be called before fit() or with a Path argument. Also widened load_default_params(params_file) to accept Path (it already did at runtime; only the annotation was wrong). Found and filed as a separate bug (#123, not fixed here): _reject_outliers() referenced self.data_mask, which was never initialized anywhere -- do_reject would crash with AttributeError if its schedule ever fired, and even fixed, nothing consumes data_mask to actually filter the fit data (unlike the torch backend's good_idx). Added a minimal, non-crashing initialization here (an all-True mask) with a comment pointing to #123; the feature is still functionally incomplete pending that issue. No behavior change otherwise; full non-slow numpy-backend test suite green.
Direct attribute access on AMICATorchNG (.A, .comp_list) and on the sklearn-wrapper's Optional .model_ after fit()/save()/load(), plus a **dict-spread into AMICATorchNG's constructor whose inferred dict[str, int | str | bool] value type doesn't match each keyword's specific type (a known false-positive pattern for spreading heterogeneous dicts into a strongly-typed constructor -- annotated the dict Dict[str, Any] instead of duplicating the kwargs). No behavior change; full file passes (19 passed).
Same recurring patterns as the other torch_tests files: direct attribute access on AMICATorchNG's Optional fit-dependent attributes, dict-spread into strongly-typed constructors (annotated Dict[str, Any] instead of duplicating kwargs), and two instance-level method monkeypatches (_finalize_newton_stats = spy) rewritten with unittest.mock.patch.object so the replacement is properly scoped and type-correct instead of a raw attribute overwrite. _snapshot_params() legitimately returns Dict[str, object] (it mixes Tensor values with a plain int), so the two tests indexing into it now isinstance-narrow before using the Tensor ops. No behavior change; full file passes (34 passed, 2 deselected).
Same recurring patterns: Optional attribute access after fit() (m.pdtype, m.W), and dict-spread into AMICATorchNG's constructor from a dynamically-keyed dict (annotated dict[str, Any] rather than restructuring the parametrization). No behavior change; full file passes (34 passed, 5 skipped).
Same pattern as the torch/numpy backends: fit-dependent attributes are None until fit() runs. Explicit Optional[mx.array] annotations (safe under `from __future__ import annotations`, so no runtime cost even when mlx isn't installed) plus assert narrowing at internal call sites. The one remaining diagnostic (unresolved-import for mlx.core) is not fixable from this repo: mlx ships mlx.core as a compiled extension (.so) with no type stubs, so no static checker can resolve it, verified by installing mlx locally (uv pip install mlx, Apple Silicon) and re-running ty -- same single diagnostic persists even with the package present. No behavior change; full mlx_tests suite green (8 passed, run locally on Apple Silicon with mlx installed).
.context/ holds point-in-time investigation/debugging artifacts (per AGENTS.md's own convention: scratch_history.md, research.md, etc.), including frozen historical reproduction scripts for closed issues explicitly marked "do not ship as-is" (e.g. issue-21's corrected_mstep_prototype.py, whose fix was already ported into the shipped backend). These are not maintained production code and forcing them to stay type-clean forever conflicts with their nature as point-in-time snapshots. Live, actively-regenerated analysis scripts (e.g. issue-27's amari_distance.py, which backs current paper/docs claims) are a different case and were already fixed on their own merits, not excluded.
Same pattern: AMICA wrapper's Optional .model_ accessed directly after fit(). No behavior change; full file passes (16 passed).
The sklearn-style wrapper's public accessors (transform, get_mixing_matrix, get_unmixing_matrix, variance_order, write_amica_output, save) already call self._check_usable(...) -- which raises if self.model_ is None -- before touching self.model_, but ty can't narrow across a method call, so each accessor gets a local assert matching that already-enforced invariant. Also widened _select_device's return type and the device parameter on __init__/load from the overly-loose `object` to the accurate Union[str, torch.device] (torch is already imported in this file, so there was no reason to avoid the precise type). No behavior change; test_amica.py + test_amica_ng_wrapper.py pass (20 passed).
Same recurring patterns: Optional final_ll_ used directly after fit(), and a dict-mixed-type list spread into AMICAMLXNG's constructor (annotated list[dict[str, Any]] instead of restructuring the parametrization). No behavior change; full mlx_tests suite green (8 passed, run locally on Apple Silicon).
Same importlib.util.spec_from_file_location None-check as the existing repo precedent, Optional final_ll_ used in float() unguarded, and an untyped heterogeneous-dict list (runs) whose "frames"/"channels" fields lost their int type through a plain dict[str, object] inference (annotated list[dict[str, Any]]). The two remaining unresolved-import diagnostics (mne) are the optional `viz` extra, not installed by default -- verified clean by installing it locally (uv pip install "mne>=1.6") and re-running ty. No behavior change; test_decompose_equiv.py (which imports from this script) passes.
Same dtype: np.dtype = np.float64/float32 default-vs-annotation mismatch already fixed once in torch_impl/utils.py, now in load_data_file/load_multiple_files (widened to npt.DTypeLike, canonicalized once via np.dtype(dtype) before use in load_multiple_files so its return type stays a concrete ndarray); load_results(indir: str) reassigned to Path(indir) internally (widened to Union[str, Path]); and the same importlib.util.spec_from_file_location None-check pattern in the test file. No behavior change; test_fortran_adapter.py, test_pyAMICA.py, test_amica.py all pass.
Optional final_ll_ used in float() unguarded (torch and mlx timing helpers), two mixed-type dict-spread helpers into AMICA/AMICATorchNG constructors (annotated dict[str, Any]), and _platform_info's dict whose value type was inferred as str from its two initial literal entries then rejected int/list[float] additions (annotated dict[str, Any]). The one remaining diagnostic (unresolved-import for mlx.core) is the same irreducible mlx stub gap documented in the mlx_impl/core.py fix commit. No behavior change; test_dimsweep_threads.py (which imports from this script) passes.
Same pattern: AMICA_NumPy's Optional .W/.A/.alpha accessed directly after fit()/load. No behavior change; 5 passed, 1 xfailed (expected).
Two spec_from_file_location None-checks (same established pattern), and load_data_file's filepath: str widened to Union[str, Path] -- open()/np.fromfile() already accept PathLike at runtime, and two call sites in the test suite already pass Path objects directly, so the annotation was simply wrong rather than the call sites. No behavior change; test_channel_selection.py, test_decompose_equiv.py, test_amica.py, test_pyAMICA.py all pass (21 passed).
Remaining 2 are the irreducible mlx.core unresolved-import (compiled .so, no .pyi stubs) in mlx_impl/core.py and benchmark_dimsweep.py.
mlx ships as a compiled extension with no type stubs, so ty cannot resolve 'import mlx.core' statically even when the package is installed (verified: the diagnostic persists after 'uv pip install mlx'). There is no annotation or assert that resolves a missing-stubs import, so scope an explicit '# ty: ignore[unresolved-import]' to those two lines only, with a comment explaining why. 'uv run ty check .' now reports 0 diagnostics repo-wide, meeting issue #122's definition of done and unblocking the CI gate in #119.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #124 +/- ##
==========================================
- Coverage 79.76% 79.60% -0.16%
==========================================
Files 14 14
Lines 2011 2064 +53
Branches 339 345 +6
==========================================
+ Hits 1604 1643 +39
- Misses 301 310 +9
- Partials 106 111 +5
🚀 New features to boost your workflow:
|
Silent-failure review found do_reject=True on the NumPy backend crashes mid-fit (num_good_samples divided every iteration but only set later in _reject_outliers) -- a bare AttributeError deep in the EM loop. fit() now refuses it with a clear NotImplementedError (issue #123) instead of crashing cryptically or silently no-op'ing; misleading data_mask comments corrected; _reject_outliers made self-contained dormant scaffolding; added a regression test. Also guard AMICATorchNG.write_amica_output (public accessor missed by the ty pass because Optionals flow through an unannotated local helper) with the same RuntimeError as its sibling accessors. Doc/comment consistency: caveat numpy outlier rejection in AGENTS.md and docs/api/numpy-backend.md (#123); reword the .context/** ty-exclude comment to name its example file and note tested scripts were fixed not excluded. Minor: lowercase dict[str, Any] in test_ng_sharing; drop redundant str() in test_amica.
Member
Author
|
The Intel benchmark host is a Core i9-13900K (24 cores / 32 threads, 8P+16E), not a Xeon. Fix paper.md Table 2 and the docs core-scaling section (header, the "24 of 32 cores" and "32-core workstation" phrasings that conflated threads with cores). Rebuild and commit paper.pdf (JOSS inara toolchain) so the compiled paper lives in the repo root.
neuromechanist
added a commit
that referenced
this pull request
Jul 14, 2026
Port AMICATorchNG's outlier rejection to AMICA_NumPy, replacing the non-functional data_mask stub. Previously do_reject was dead: _reject_outliers thresholded the scalar total LL (not per-sample), self.data_mask was never consumed by the E-step, and num_good_samples was used before assignment. - good_idx: an index array of kept samples (init to all in _initialize_parameters, only ever shrinks), with num_good_samples tracking its size. - _get_updates_and_likelihood restricts the E-step to data[:, good_idx] and captures the pre-update per-sample LL (in good_idx order) when do_reject. - _reject_outliers thresholds that per-sample LL (mean - rejsig*std, population std), shrinks good_idx, and raises a clear ValueError if all samples drop (mirrors the torch backend exactly). - gm/LL normalize by num_good_samples under do_reject. - Validate rejint/rejsig/maxrej at construction (matching AMICATorchNG). do_reject=False is bit-identical: every addition is guarded by self.do_reject and the LL split is np.sum of the same expression. Removes the #124 NotImplementedError gate. New test_numpy_reject.py exercises the real sample EEG (shrinks good set, gm stays a valid distribution, degenerate LL raises, param validation); drops the now-obsolete refusal test.
neuromechanist
added a commit
that referenced
this pull request
Jul 14, 2026
Reverse the #124 caveats in AGENTS.md and docs/api/numpy-backend.md now that outlier rejection is implemented on the NumPy backend.
neuromechanist
added a commit
that referenced
this pull request
Jul 14, 2026
* Implement NumPy do_reject via good_idx (#123) Port AMICATorchNG's outlier rejection to AMICA_NumPy, replacing the non-functional data_mask stub. Previously do_reject was dead: _reject_outliers thresholded the scalar total LL (not per-sample), self.data_mask was never consumed by the E-step, and num_good_samples was used before assignment. - good_idx: an index array of kept samples (init to all in _initialize_parameters, only ever shrinks), with num_good_samples tracking its size. - _get_updates_and_likelihood restricts the E-step to data[:, good_idx] and captures the pre-update per-sample LL (in good_idx order) when do_reject. - _reject_outliers thresholds that per-sample LL (mean - rejsig*std, population std), shrinks good_idx, and raises a clear ValueError if all samples drop (mirrors the torch backend exactly). - gm/LL normalize by num_good_samples under do_reject. - Validate rejint/rejsig/maxrej at construction (matching AMICATorchNG). do_reject=False is bit-identical: every addition is guarded by self.do_reject and the LL split is np.sum of the same expression. Removes the #124 NotImplementedError gate. New test_numpy_reject.py exercises the real sample EEG (shrinks good set, gm stays a valid distribution, degenerate LL raises, param validation); drops the now-obsolete refusal test. * Docs: NumPy do_reject is now functional (#123) Reverse the #124 caveats in AGENTS.md and docs/api/numpy-backend.md now that outlier rejection is implemented on the NumPy backend. * Address PR #126 review: reject schedule, restart, LL comments Code review found the reject schedule missing Fortran/torch's max(1, ...) clamp: Python's non-negative modulo let (iter-rejstart) % rejint hit 0 for iter < rejstart, firing rejection before rejstart. Add the clamp (verified: rejstart=3,rejint=2 now fires at [3,5,7], not [1,3,5]). Silent-failure + comment review found my "num_good_samples drives the gm/LL normalization" comments wrong: self.ll is a raw sum over the good set, never normalized (this backend's convention for both modes). Correct the comments to say only gm is normalized, and note that dropping the most-negative samples raises the raw-sum LL, so the reject iteration is an increase and does not spuriously trip the convergence checks (now covered by a test). Also: expose self.numrej as an instance attribute (matching AMICATorchNG) so the maxrej cap is testable; preserve good_idx/num_good_samples across a NaN restart (Fortran startover keeps prior rejections); validate rejstart>=0; fix the rejsig<=0 validation wording (at 0 it keeps ~half, not "every sample"). Tests: assert the numrej cap and c-finiteness under multi-model rejection (dead-model dgm==0 guard), add an LL-sanity test across the reject event.
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
2 participants
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.
Summary
Fixes #122. Drives
uv run ty check .from 496 diagnostics to 0repo-wide, unblocking the CI gate + pre-commit hooks tracked in #119.
The large majority of diagnostics had a single root cause: the stateful
estimators (
AMICATorchNG,AMICA_NumPy,AMICAMLXNG) initializefit-dependent attributes (
self.W,self.A,self.sphere,self.c,self.comp_list, ...) toNonein__init__and only assign realtensors/arrays in
fit().tycorrectly infers these asTensor | None(etc.) and flags every downstream use.
Fix approach (no API changes, no blanket silencing)
assert x is not Noneat point of use,treating "fit populated this" as an enforced invariant ("trust internal
code", per CLAUDE.md).
transform,get_mixing_matrix,get_unmixing_matrix,variance_order,save, ...): realRuntimeErrorguards, matching the existingstate_dict()/_check_usableconvention, since a user can call these before
fit().assertright after the relevantfit()/helper call,which also documents "fit succeeded" as a precondition.
Optional[...]annotations added to the fit-dependentattributes so the intent is declared, not just inferred.
spec_from_file_location(...) -> ModuleSpec | Nonenarrowing in testloaders,
np.dtypedefault/annotation mismatches widened tonpt.DTypeLike,str-only path params widened toUnion[str, Path],dict-spread-into-typed-constructor annotated
dict[str, Any], and rawinstance-attribute method monkey-patches replaced with
mock.patch.object(...).Scoped exceptions (documented, not silenced globally)
.context/**excluded fromtyscope via[tool.ty.src]inpyproject.toml: these are frozen historical investigation scripts forclosed issues (explicitly marked "do not ship as-is"). Live/regenerated
analysis scripts were fixed on their own merits, not excluded.
import mlx.corelines carry a scoped# ty: ignore[unresolved-import]with an explanatory comment.mlxships as a compiled extension with no
.pyistubs, sotycannotresolve the import even when the package is installed (verified: the
diagnostic persists after
uv pip install mlx). No annotation or assertcan resolve a missing-stubs import; the suppression is local to those two
lines only.
Side-effect finding
While annotating
numpy_impl/core.pyI found_reject_outliers()references
self.data_mask, which was never initialized anywhere, sodo_reject=Truewould crash. Filed as #123. This PR adds a minimalnon-crashing init (
data_maskallocated in_initialize_parameters) witha comment noting the feature is still functionally incomplete pending the
real fix in #123 (out of scope here).
Validation
uv run ty check .-> All checks passed! (0 diagnostics)uv run ruff check/ruff format --checkcleanDefinition of done (#122)
uv run ty check .reports 0 diagnosticsty checkinto CI as a hard gate + pre-commit config -> CI: enforce ty type-checking + add pre-commit config (ruff + ty) #119