Skip to content

Fix LRR apply: detect block-diagonal clusters from the weight matrix - #20

Merged
cboulay merged 1 commit into
devfrom
fix/lrr-noncontiguous-cluster-apply
Aug 7, 2026
Merged

Fix LRR apply: detect block-diagonal clusters from the weight matrix#20
cboulay merged 1 commit into
devfrom
fix/lrr-noncontiguous-cluster-apply

Conversation

@kylmcgr

@kylmcgr kylmcgr commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

LRRTransformer could silently apply the wrong rereference when the fitted/loaded weight matrix is block-diagonal over clusters that don't match the transformer's block_size — most notably when cluster_by_field groups channels non-contiguously (e.g. an electrode array split across two connector banks). The decode/output looked valid but each block was rereferenced against only a subset of its channels.

Root cause

_on_weights_updated built the internal AffineTransform with channel_clusters=self._get_channel_clusters(n). That call runs when the weights are set — at construction for a pre-fit/loaded W, before any message has arrived — so cluster_by_field hasn't been resolved yet and it falls back to block_size clusters. When those fallback clusters are finer than the W's real blocks, two input sub-groups of one true block resolve to the same output indices. The affine's block-diagonal matmul writes with assignment (result[out_idx] = chunk @ subW, not +=), so the second sub-group silently overwrites the first — leaving each block rereferenced against only part of its channels. (Contiguous / matching clusters, e.g. bank-aligned, were unaffected, which is why this went unnoticed.) The cluster hint is only a performance optimization; it is not needed for correctness, since the weight matrix already encodes its own block structure.

Fix

Pass channel_clusters=None so the affine auto-detects the block-diagonal structure directly from the weight matrix — the single source of truth — removing any fit/apply cluster mismatch. Auto-detection runs once at first build; adaptive weight updates reuse it via the in-place set_weights path, so there's no added per-update cost and the block-diagonal speedup is retained.

Testing

New regression test TestApplyFollowsWeightBlocks: fits a W over two non-contiguous 64-ch groups, applies it with a mismatched block_size=32, and asserts the output equals X @ (I − W). Fails on main, passes with this change. Full tests/unit/test_ssr.py suite (41 tests) passes.

Follow-up?

ezmsg-sigproc's AffineTransform will silently mis-computes for any caller that passes channel_clusters not matching the weights' nonzero structure. Should we add a PR there to validate/auto-correct/raise on a cluster–weight mismatch?

@kylmcgr
kylmcgr requested review from cboulay and a lite review from Copilot August 5, 2026 22:44
@kylmcgr kylmcgr self-assigned this Aug 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a correctness bug in LRRTransformer where applying pre-fit/loaded weights could silently produce an incorrect rereference when the transformer’s cluster hint (derived from block_size / unresolved cluster_by_field) didn’t match the true block-diagonal structure encoded in the weight matrix.

Changes:

  • Build the internal AffineTransformTransformer without passing channel_clusters, so block structure is derived from the weight matrix itself.
  • Add a regression test covering non-contiguous clusters with a mismatched (finer) block_size, asserting apply matches X @ (I - W).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/ezmsg/learn/process/ssr.py Stops passing potentially incorrect cluster hints when constructing the affine transform from weights, preventing silent overwrite in block-apply optimization.
tests/unit/test_ssr.py Adds a regression test ensuring apply follows the weight matrix’s true block structure even when block_size is incompatible.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kylmcgr
kylmcgr marked this pull request as ready for review August 6, 2026 16:29
@kylmcgr

kylmcgr commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@cboulay ready for review

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.

3 participants