Skip to content

fix: _validate_seqpos rejects valid step_size of 1 in seqpos_slice#681

Open
robbiebusinessacc wants to merge 1 commit into
decoderesearch:mainfrom
robbiebusinessacc:contrib/fix-seqpos-step-size-validation
Open

fix: _validate_seqpos rejects valid step_size of 1 in seqpos_slice#681
robbiebusinessacc wants to merge 1 commit into
decoderesearch:mainfrom
robbiebusinessacc:contrib/fix-seqpos-step-size-validation

Conversation

@robbiebusinessacc

Copy link
Copy Markdown
Contributor

_validate_seqpos in sae_lens/config.py validates the step component of a
3-tuple seqpos_slice. The intent (per the comment "Ensure that the step-size
is larger or equal to 1" and the error message "is at least 1") is to reject
steps below 1. But the guard was written as step_size <= 1, which also
rejects a step of exactly 1 — the most common case.

Because the step defaults to 1 when omitted (step_size = seqpos[2] or 1),
this made every natural 3-tuple usage crash at config-construction time:

  • seqpos_slice=(0, 5, 1) — explicit step of 1
  • seqpos_slice=(None, 5, None) — step omitted, defaults to 1
  • seqpos_slice=(1, None, 1) — e.g. drop the BOS position, keep the rest

All raised ValueError: Ensure the step_size=1 for sequence slicing is at least 1. — a self-contradictory message, since 1 is at least 1.

Fix

Relax the guard so that a step of 1 is accepted — an omitted or None step
defaults to 1 as well — while 0 and negative values are still rejected as
before. It is a one-character change in sae_lens/config.py, replacing the
comparison step_size <= 1 with step_size < 1.

Testing

The existing parametrized tests only covered error cases (steps of 0, -1, and
empty/reversed ranges); the valid step == 1 path was untested. This PR adds
parametrized tests for both LanguageModelSAERunnerConfig and
CacheActivationsRunnerConfig asserting that valid slices such as (0, 5, 1), (None, 5, None), (None, None, 1), and (1, None, 1) are accepted and
round-trip correctly.

Reverting only the one-character fix makes 8 of the new tests fail; restoring
it makes all pass. ruff check, ruff format --check, and pyright are all
clean.

The step-size check used `if step_size <= 1`, which rejected the valid
step of 1 even though the comment and error message both state the step
must be at least 1. This made any 3-tuple seqpos_slice with an explicit
or defaulted step of 1 (e.g. (0, 5, 1), (1, None, 1), (None, None, 1))
crash at config construction with a self-contradictory error.

Change the check to `< 1` so step 1 is accepted while 0 and negative
steps still raise. Add tests covering valid 3-tuple slices that were
previously untested.
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