Skip to content

Fix comp_used staleness and NaN under sharing - #243

Merged
neuromechanist merged 2 commits into
devfrom
240-numpy-share-comps
Aug 16, 2026
Merged

Fix comp_used staleness and NaN under sharing#243
neuromechanist merged 2 commits into
devfrom
240-numpy-share-comps

Conversation

@neuromechanist

Copy link
Copy Markdown
Member

Closes #240.

Root cause

identify_shared_components rebuilt comp_used = np.ones(...) on every call. The merge loop skips any pair where k1 == k2, so once comp_list is already merged, no merge fires and the mask comes back all-True while half the columns are dead.

The unmasked mixture update then divided 0/0 for every merged-away column:

mu:   nan=96/192
beta: nan=96/192
comp_used: 64 of 64      <-- claims every column is live

and the fit returned normally.

Fix

comp_used is now derived from the final comp_list — exactly the set of referenced columns — matching AMICATorchNG.comp_used, which is a property derived the same way and so cannot go stale.

The mixture update now skips unused columns and freezes them at their last finite value, as the torch backend already did. Fortran carries NaN there harmlessly behind its own comp_used mask; keeping them finite means a fit cannot report success while holding NaN parameters.

mu:   nan=0/192
beta: nan=0/192
comp_used: 32 of 64

np.errstate guards the discarded branch of each np.where, so the 0/0 that is computed and thrown away no longer warns every iteration.

Parity

Default (disjoint comp_list) is bit-identical for single- and multi-model — verified before/after on real sample EEG. Every change is behind the used mask, which is all-True without a merge.

Tests

pamica/tests/test_numpy_share_comps.py, 5 tests. Verified by reverting comp_used to np.ones(...): test_comp_used_matches_the_columns_comp_list_references and test_sharing_leaves_finite_mixture_parameters both fail.

What is deliberately not here

#242 — the shared-column A-update, where numpy takes one step per contributing model instead of Fortran's single weighted application — is not fixed in this PR.

I implemented it (a one-line change to use the dAk already computed above, identical to the torch backend), then wrote three tests for it: a magnitude comparison against private columns, a cross-backend comparison against AMICATorchNG, and a finiteness check. All three passed equally with the per-model loop and with the single weighted application — none distinguished the two. The magnitude test's premise was simply wrong: a merged column legitimately moves ~3x a private one, because the averaged direction need not be small.

Rather than ship a numerical change to the A-update with tests that give false confidence — the exact failure caught in #241's review — the change is reverted and #242 stays open until it has a test that fails on revert. The comment at the loop records why.

@neuromechanist

Copy link
Copy Markdown
Member Author

Review findings addressed

The reviewer ran the revert matrix per change. Unlike #241, this PR's tests do go red on revert — but it found a subtler version of the same blind spot, plus a real gap I introduced.

1. Two tests skipped rather than failed under the bug. Their guards (if used_first.all(): skip, if not unused.any(): skip) were keyed to precisely the symptom the bug produces — a stale all-True mask reads as "no merge fired". So under the bug they silently opted out instead of failing. Same blind spot as #241, wearing a skip instead of a pass.

Fixed by forcing the merge deterministically (_force_collinear_pair copies one model's column onto another's, so a merge must fire) and turning both guards into assertions. Revert matrix now: 4 of 5 fail, where before it was 2 failing and 2 skipping.

2. alpha lost its only signal. There is no np.isfinite(self.alpha) check anywhere in the file — only mu/beta have one. My np.errstate silences the discarded 0/0 for dead columns, but it equally silenced numpy's warning for a genuine 0/0 in a live column, which was the sole indication it had happened. The NaN would still surface eventually through the nan-LL restart, but with the origin lost — cutting against this same function's stated philosophy for the rho reset two blocks down.

Added an explicit alpha finiteness check with a warning, mirroring the mu/beta canary.

3. The freeze comment overclaimed. It said unused columns are frozen at their last finite value, but rho is excluded — its own 1e-8 floor keeps it finite, so it drifts rather than freezing. Harmless, since no dead column is read downstream, but the comment now says so.

Confirmed, not changed

  • comp_used derivation is correct on first call, on repeat calls, and for never-merged columns — comp_list starts as a bijection and merges only overwrite, so np.unique always recovers the live set.
  • Freezing vs Fortran's NaN-tolerant mask is inert: get_unmixing_matrices and the A-update both index through comp_list, which never references a merged-away column.
  • The fix silently corrects a second consumer for free: nd_value reads comp_used and was over-counting its denominator with dead columns after a merge, deflating the reported gradient norm.

Two low-confidence notes I am leaving as-is: a num_comps > data_dim * n_models edge case inherited verbatim from AMICATorchNG.comp_used, and the absence of a cross-backend comp_used agreement test. Neither is introduced here; the second is worth adding when #242 lands, since that PR needs a cross-backend test anyway.

@neuromechanist
neuromechanist merged commit ff3ea7b into dev Aug 16, 2026
7 checks passed
@neuromechanist
neuromechanist deleted the 240-numpy-share-comps branch August 16, 2026 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant