Conversation
stack_neuron_results wrapped neuron_slice with Slice(), so an int (a valid SliceInput) collapsed the neuron axis. Building the labels then raised "TypeError: iteration over a 0-d tensor", and that crash hid two more bugs: the non-LN path concatenated layers along positions (e.g. (8, 3, 16) instead of (2, 3, 4, 16)), and the LN-folded projection path raised IndexError (LN) or a broadcast RuntimeError (RMS). Normalise with Slice.unwrap() so an int keeps the neuron axis like [n] on every path. This makes the isinstance(neuron_labels, int) guard dead, so it is removed along with the now-unused numpy import. Add a regression test comparing neuron_slice=n against neuron_slice=[n] (labels, shape and values) for apply_ln on/off, no/vector/matrix project_output_onto, and incl_remainder on/off, on LN and RMS models. Fixes TransformerLensOrg#1841
| neuron_slice = Slice(neuron_slice) | ||
| # unwrap turns an int into [n]: layers are concatenated along the neuron axis, so it | ||
| # must not collapse. | ||
| neuron_slice = Slice.unwrap(neuron_slice) |
There was a problem hiding this comment.
Thanks for addressing this follow-up to #1836/#1837 and adding coverage for the different normalization and projection paths!
I noticed one remaining edge case: the signature accepts neuron_slice=Slice(n), but Slice.unwrap leaves existing Slice instances unchanged. Since Slice(n) remains in integer mode, applying it still collapses the neuron axis. This makes neuron_labels a 0-D tensor, so the unconditional label comprehension raises the original TypeError even when return_labels=False.
Could we normalize integer-mode Slice inputs locally in stack_neuron_results and extend the regression test to verify that Slice(neuron) behaves the same as [neuron]? Keeping the normalization local would preserve the dimension-collapsing semantics of Slice/get_neuron_results elsewhere.
This looks like an existing uncovered edge case rather than a regression introduced by this PR. My review is based on static code inspection only; I haven’t run the tests locally.
There was a problem hiding this comment.
Thanks, good catch. Slice.unwrap passes an existing Slice through unchanged, so Slice(n) still collapsed the neuron axis and the label comprehension failed even with return_labels=False.
Fixed in a follow-up commit. After unwrap, stack_neuron_results now rebuilds an integer-mode Slice as a new Slice([n]). The normalization stays local: the caller's object is not modified, and Slice / get_neuron_results keep their dimension-collapsing int semantics.
Tests: 3 and Slice(3) are now both compared against [3] (labels, shape, values) over the same matrix, plus a return_labels=False case; the 26 new Slice(3) cases fail on the previous commit with TypeError: iteration over a 0-d tensor and pass now, and the whole tests/unit/test_activation_cache.py passes (108).
AI assistance: drafted with an AI coding assistant (Claude).
Slice.unwrap() only converts a bare int into [n]; an existing Slice is returned unchanged. So neuron_slice=Slice(n) stayed in integer mode, collapsed the neuron axis, and building the labels raised "TypeError: iteration over a 0-d tensor", also with return_labels=False. Rebuild an integer-mode Slice as a new Slice([n]) inside stack_neuron_results only. The caller's Slice is not modified, and Slice / get_neuron_results keep their dimension-collapsing int semantics. Extend the regression test so Slice(n) is compared against [n] (labels, shape, values) over the same matrix as the bare int, and add a return_labels=False case for both forms.
yuanwuyuan9
left a comment
There was a problem hiding this comment.
Thanks for the update! I’ve reviewed the follow-up change, and it addresses the Slice(n) edge case I raised.
Rebuilding the slice locally preserves the caller’s object and the existing semantics elsewhere, and the added coverage includes return_labels=False.
No further concerns from my static review; I haven’t run the tests locally.
|
Looks good to me as well! Merging as is! Thanks @Zhuoxi2000 |
Base branch:
devDescription
ActivationCache.stack_neuron_results(layer, neuron_slice=n)with an intn(a validSliceInput) wrapped the input withSlice(n), which collapses the neuron axis. The visible symptom wasTypeError: iteration over a 0-d tensorwhile building the labels. That crash hid two more bugs further down:(8, 3, 16)instead of(2, 3, 4, 16). Withapply_lnorincl_remainderit then fails with a broadcastRuntimeError.apply_ln=Truewithproject_output_onto):IndexErroron LN models and a broadcastRuntimeErroron RMS models.Fix: normalize with
Slice.unwrap(neuron_slice), which turns an int into[n]. The neuron axis is then kept on every path (raw,apply_ln,project_output_onto, the folded LN+projection path, andincl_remainder), and the result matchesneuron_slice=[n]. This also makes theisinstance(neuron_labels, int)guard dead, so it is removed together with the now-unusednumpyimport.Slice.unwrapreturns an existingSliceunchanged, so an integer-modeSlice(n)is also rebuilt as a newSlice([n])insidestack_neuron_results. The caller's object is not modified, andSlice/get_neuron_resultskeep their dimension-collapsing int semantics. Theneuron_slicedocstring says that an int or an integer-modeSlice(n)is treated like[n].Fixes #1841
Type of change
Test evidence
New tests in
tests/unit/test_activation_cache.py:test_stack_neuron_results_integer_neuron_slice_matches_listrunsstack_neuron_results(n_layers, ...)withneuron_slice=3and withneuron_slice=Slice(3), and compares each againstneuron_slice=[3]: the labels are equal, the shapes are equal, andtorch.testing.assert_closepasses on the values. It also checks that the stack has one row per layer (plus the remainder). The matrix is:neuron_sliceform: bare int3/Slice(3)apply_ln: off / onproject_output_onto: none /[d_model]vector /[d_model, 2]matrixincl_remainder: off / ond_model=16, built natively, no download)That gives 48 cases.
test_stack_neuron_results_integer_neuron_slice_without_labelsdoes the same shape and value comparison withreturn_labels=False, for3andSlice(3)on both models (4 cases), because the labels were built even when they were not returned.dev@ cfac4beTypeError: iteration over a 0-d tensordev@ 02a7f5a + label guard patched only (0-d tensor reshaped,Slice(n)kept), bare-int matrix only(8, 3, 16) != (2, 3, 4, 16); 4:IndexError(LN, folded projection); 14: broadcastRuntimeError(remainder subtraction,apply_ln_to_stack, RMS folded projection)Slice(3)cases:TypeError: iteration over a 0-d tensorThe label-guard row comes from a throwaway patch and is not part of this PR. It shows that the test checks the result against
[n]and does not only check that the call no longer raises.Whole file:
uv run pytest transformer_lens/ActivationCache.py transformer_lens/utilities/slice.py(doctests): 5 passed.Lint, using the versions from
uv.lock(black 23.12.1, isort 5.8.0, pycln 2.5.0, mypy 1.17.0): themake check-formatcommands (pycln / isort / black, repo-wide) are clean, anduv run mypy .reports no issues in 387 source files.Checklist:
AI assistance: this change was drafted with an AI coding assistant (Claude) and verified locally with the tests above.