fix: prevent singleton shrinkage chunks from modifying covariance - #28
Open
Tal-Golan wants to merge 2 commits into
Open
fix: prevent singleton shrinkage chunks from modifying covariance#28Tal-Golan wants to merge 2 commits into
Tal-Golan wants to merge 2 commits into
Conversation
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
marked this pull request as ready for review
August 19, 2026 22:49
Member
|
Thanks Tal. @jacob-prince is busy at the moment but will get to this next week. |
Collaborator
|
yes thanks @Tal-Golan for raising this! i'll handle asap. |
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
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
Fix an in-place modification in the Torch shrinkage-NLL path that can cause
GSN to select
lambda = 0even 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:
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 withc_t.The following in-place multiplication then modifies
c_t. Later shrinkagelevels 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:
test: show that one shrinkage level changes the input covarianceAdds 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.
fix: copy the covariance before changing itReplaces
expand(...).contiguous()with an explicit contiguous clone:This guarantees independent writable storage before the in-place
multiplication. The test introduced by the first commit passes after this
change.
Validation
145 passed, 2 skipped.