Skip to content

fix: prevent singleton shrinkage chunks from modifying covariance - #28

Open
Tal-Golan wants to merge 2 commits into
cvnlab:mainfrom
Tal-Golan:fix/one-level-input-mutation
Open

fix: prevent singleton shrinkage chunks from modifying covariance#28
Tal-Golan wants to merge 2 commits into
cvnlab:mainfrom
Tal-Golan:fix/one-level-input-mutation

Conversation

@Tal-Golan

Copy link
Copy Markdown

Summary

Fix an in-place modification in the Torch shrinkage-NLL path that can cause
GSN to select lambda = 0 even when it is not optimal.

I encountered this with a large covariance matrix when the available memory
caused the automatic batching code to process one shrinkage level at a time.

Cause

The code intends to create a writable copy before modifying each chunk:

covs = c_t.unsqueeze(0).expand(chunk_size, N, N).contiguous()
covs.mul_(alphas_chunk[:, None, None])

For chunks containing multiple levels, contiguous() creates a new tensor.
For a singleton chunk, however, the expanded view may already be contiguous,
so contiguous() can return storage shared with c_t.

The following in-place multiplication then modifies c_t. Later shrinkage
levels are evaluated using this modified tensor, which can incorrectly make
the selection fall back to lambda = 0.

Singleton chunks typically occur when the available memory is low relative to
the covariance matrix size.

Commits

This PR contains two commits:

  1. test: show that one shrinkage level changes the input covariance

    Adds a focused test that demonstrates the issue using a singleton shrinkage
    level. The test shows that the Torch path modifies the input covariance and
    also compares its NLL with the NumPy implementation. It fails before the
    fix.

  2. fix: copy the covariance before changing it

    Replaces expand(...).contiguous() with an explicit contiguous clone:

    covs = c_t.unsqueeze(0).expand(chunk_size, N, N).clone(
        memory_format=torch.contiguous_format
    )

    This guarantees independent writable storage before the in-place
    multiplication. The test introduced by the first commit passes after this
    change.

Validation

  • The focused test fails on the first commit and passes on the second.
  • Complete suite: 145 passed, 2 skipped.

To control memory use, the Torch path automatically splits the requested shrinkage levels into groups. A group can contain only one level, even when the user requested many levels. For a one-level group, expand(...).contiguous() may reuse the input covariance's memory instead of creating new memory. The following in-place multiplication can then change the input covariance. This test uses one level to reproduce that case reliably and checks that the input stays unchanged and the result matches NumPy.
Automatic batching can create a group containing only one shrinkage level. For that group, contiguous() may leave the expanded covariance sharing memory with the input covariance, so the following in-place multiplication changes the input. Clone the expanded covariance before scaling it so every group has its own memory.
@Tal-Golan
Tal-Golan marked this pull request as ready for review August 19, 2026 22:49
@kendrickkay

Copy link
Copy Markdown
Member

Thanks Tal. @jacob-prince is busy at the moment but will get to this next week.

@jacob-prince

Copy link
Copy Markdown
Collaborator

yes thanks @Tal-Golan for raising this! i'll handle asap.

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