fix(rag)!: preserve string boundaries when indexing a string-under-axis Ragged - #72
Merged
Merged
Conversation
Integer indexing on a string-under-axis Ragged concatenated the whole group into one bytes, dropping the per-string boundaries already held in str_offsets. It now peels to the standalone opaque-string layout (k,), so len(s[i]) == s.lengths[i] and s[i][j] is one string. BREAKING CHANGE: Ragged.__getitem__ with an integer on a string-under-axis array returns a Ragged of strings, not one concatenated bytes. Use b"".join(s[i]) for the old value. Refs #71
_getitem_record_rows returned a string field as the raw concatenated S1 buffer, so a peeled row mixed a 3-char array with a 2-element numeric array. String fields now peel to the standalone opaque-string layout, giving every field of the row the same length. BREAKING CHANGE: peeling a record row returns opaque-string fields as a Ragged of strings, not a concatenated S1 array. Refs #71
Contributor
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_bench_ragged_cres |
978.1 µs | 1,465.7 µs | -33.27% |
| ⚡ | test_bench_baseline_ragged_short_alleles |
4.2 ms | 2.7 ms | +57.94% |
| ⚡ | test_bench_baseline_ragged_cres |
8.2 ms | 6.2 ms | +33.01% |
| ⚡ | test_bench_dense_batch |
3.4 ms | 2.9 ms | +17.24% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing worktree-issue-71-string-under-axis-getitem (e970469) with main (c7f4a5a)
d-laub
marked this pull request as ready for review
July 27, 2026 18:18
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.
Closes #71.
Integer indexing on a string-under-axis
Raggedconcatenated a whole group into one blob, dropping the per-string boundaries already sitting instr_offsets.s.lengths[0] == 2butlen(s[0]) == 3.Two sites, not one
Ragged.__getitem__integer branch — the one reported._getitem_record_rowsinteger branch — worse, and the one GenVarLoader actually hits.RaggedVariantsis a recordRaggedwhosealt/refare opaque-string fields sharing offsets with numericstart/ilen, so a peeled row returned a concatenatedS1char array (not evenbytes) sitting next to a per-variant numeric array in the same dict:_getitem_record_rows_r2delegates to the first site and inherits the fix; it's covered by a test.Fix
Both now peel to the standalone opaque-string layout (
offsets == [],str_offsetsset,shape == (k,)), which Spec C already defines as the zero-real-level special case of string-under-axis — so this is what "peel one real level" already means in the layout algebra, not a new return type. Shared helper_peel_string_row, mirroring the narrowing_slice_contig_stringalready does for slices. Data stays zero-copy; only the small(k+1,)offsets slice is copied, rebased to zero asis_contiguousrequires.Result:
This makes
zip(rv.start[0][h], rv.alt[0][h])correct in GenVarLoader (mcvickerlab/GenVarLoader#330).Alternatives considered and rejected in the spec: an object array of
bytes(same ergonomics but allocateskPython objects per row access, against the repo's hot-path rule),to_chars()[i](forces consumers to re-assemble strings), and raisingNotImplementedError(indexing is far more central than theto_numpy()/to_packed()that set that precedent).Breaking
The integer index returns a
Raggedof strings rather than one concatenatedbytes.b"".join(s[i])recovers the old value.major_version_zerois set, so this bumps 0.21.2 → 0.22.0.test_string_under_axis_integer_indexpinned the old behavior but used exactly one string per group, so it could never distinguish the two interpretations — that's why the bug survived. It has been rewritten.Testing
16 new/rewritten tests: the issue repro, numeric/string length parity, multi-dim
(batch, ploidy, ~variants), record rows with a multi-byte (indel) entry so 1-byte coincidence can't mask a regression, zero-copy, empty group,to_chars()agreement, negative/OOB indexing, and a regression guard that the standalone string case still returnsbytes.25 of 26 test files pass.
tests/test_coords.pyfails to collect withOSError: Could not find/load shared object filefromllvmlite— pre-existing environment breakage, verified identical on unmodifiedmain.Spec:
docs/superpowers/specs/2026-07-27-ragged-string-getitem-design.mdPlan:
docs/superpowers/plans/2026-07-27-ragged-string-getitem.md🤖 Generated with Claude Code