Repository navigation
refactor(concat,svar2): clear the three follow-ups from the analytic run planning review - #415
Merged
Merged
Conversation
tests/dataset/test_svar2_readbound_tracks.py duplicated the reference and VCF from tests/conftest.py's _SVAR2_SLOT_REF / _SVAR2_SLOT_VCF byte for byte, and rebuilt a .svar2 store with parameters identical to the one phased_svar2_gvl built inline. The same store was therefore produced twice per session, by two modules, from two copies of the same inputs. Extract it as a session-scoped `svar2_slot_store` fixture in tests/conftest.py and have both consume it. One genoray conversion subprocess per session instead of two, and one copy of the fixture bytes instead of three. The test module uses the fixture under its own name rather than aliasing it to `svar2_store`: it lives under tests/dataset/, where tests/dataset/conftest.py defines a DIFFERENT 3-sample `svar2_store`, and an alias would have silently bound to that instead. Verified with --fixtures-per-test that the tests resolve to tests/conftest.py's fixture. Closes #411. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VxRqNngU7Eg1wdb1aEgD6
The fixture was module-scoped because test_fingerprint_detects_mutated_store appended a byte to a .bin inside the shared store to prove the fingerprint catches tampering. Under session scope that corruption leaked into every later consumer: 17 failures plus a Rust cast_slice -> OutputSliceWouldHaveSlop panic. That test now copies the store into tmp_path and mutates the copy, so nothing mutates the shared store any more and the scope can be widened. Measured with --setup-show across the six consumer modules: 6 fixture builds under module scope, 1 under session scope. Each build runs genoray's conversion pipeline in a subprocess. Full tree re-run (not a scoped run -- this failure mode surfaces in other modules): 1265 passed, 61 skipped, 4 xfailed, 0 failed. The fixture docstring now states the read-only contract and why, so the next test that wants to mutate it copies first instead of rediscovering the panic. Closes #412. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VxRqNngU7Eg1wdb1aEgD6
copy_runs called np.unique(ds_vec) once per batch. On the samples axis that is pure waste: RunPlan.slot_batches computes ds_vec once, outside its region loop, and yields the same array object for every region, so np.unique re-sorted an identical array R times -- 3,734 sorts of a 1.07e6-element array at the All of Us chr22 grid. slot_batches now yields the distinct source indices as a fourth field. It knows them for free: a regions-axis batch and an ExplicitRunPlan batch are each single-sourced, and on the samples axis the grouping is loop-invariant. That also makes a single-source fast path available in copy_runs, which skips building an all-True mask over as many as _SLOT_BATCH_SLOTS elements. Measured on the offsets loop, outputs asserted identical to the previous implementation: samples axis, 19.2e6 slots, 2 interleaved datasets: 0.268s -> 0.185s regions axis, 9.6e6 slots, block concatenation: 0.119s -> 0.049s The new field is pinned by the existing provenance oracle test, which now also asserts it equals np.unique(ds_vec) -- copy_runs trusts it to choose which file to read offsets from, so a wrong value would be a silent wrong merge. The single-source fast path was mutation-checked: zeroing its length computation fails three concat tests. Full tree: 1265 passed, 61 skipped, 4 xfailed, 0 failed. Closes #413. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VxRqNngU7Eg1wdb1aEgD6
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 #411. Closes #412. Closes #413.
The three follow-ups filed during the final review of #414. Independent of each
other; split into one commit apiece.
#411 — fold the readbound-tracks fixture onto a shared store
tests/dataset/test_svar2_readbound_tracks.pyduplicated_SVAR2_SLOT_REF/_SVAR2_SLOT_VCFbyte for byte (verified by diff — only the constant namediffered), and rebuilt a
.svar2store with parameters identical to the onephased_svar2_gvlbuilt inline. The same store was produced twice per session,by two modules, from two copies of the same inputs.
Extracted as a session-scoped
svar2_slot_storefixture that both consume.The test module uses the fixture under its own name rather than aliasing it to
svar2_store: it lives undertests/dataset/, wheretests/dataset/conftest.pydefines a different 3-sample
svar2_storethat an alias would silently havebound to. Confirmed with
--fixtures-per-testthat the tests resolve totests/conftest.py:180.#412 — promote
svar2_store_2sto session scopeIt was module-scoped because
test_fingerprint_detects_mutated_storeappended abyte to a
.bininside the shared store; under session scope that corruptionleaked into every later consumer (17 failures plus a Rust
cast_slice -> OutputSliceWouldHaveSloppanic). That test now copies intotmp_pathand mutates the copy, so the scope can widen.Measured with
--setup-showacross the six consumer modules: 6 fixture buildsunder module scope, 1 under session scope. Each build runs genoray's conversion
pipeline in a subprocess.
Verified against the full tree, not a scoped run — this failure mode surfaces
in other modules.
The fixture docstring now states the read-only contract and why, so the next test
that wants to mutate it copies first instead of rediscovering the panic.
#413 — hoist
np.uniqueout of the per-batch loopcopy_runscallednp.unique(ds_vec)once per batch. On the samples axisRunPlan.slot_batchescomputesds_veconce, outside its region loop, and yieldsthe same array object for every region — so
np.uniquere-sorted an identicalarray R times (3,734 sorts of a 1.07e6-element array at the chr22 grid).
slot_batchesnow yields the distinct source indices as a fourth field. It knowsthem for free: regions-axis and
ExplicitRunPlanbatches are each single-sourced,and on the samples axis the grouping is loop-invariant. That also enables a
single-source fast path in
copy_runsthat skips building an all-True mask overas many as
_SLOT_BATCH_SLOTSelements.Measured on the offsets loop, outputs asserted identical to the previous
implementation:
The new field is pinned by the existing
provenanceoracle test, which now alsoasserts it equals
np.unique(ds_vec)—copy_runstrusts it to choose which fileto read offsets from, so a wrong value would be a silent wrong merge rather than a
crash. The fast path was mutation-checked: zeroing its length computation fails
three concat tests.
Testing
pixi run -e dev pytest tests -q— 1265 passed, 61 skipped, 4 xfailed, 0 failedruff check/ruff formatclean;pyrefly0 errors (47 suppressed, 596 warnings — non-vacuous)api.mdin sync with__all__(MISSING: none); no public API touched, no Rust touchedNot done here
tests/still holds 7 copies of the same 40 bp reference string. Only thereadbound-tracks module matched both the reference and the VCF, so it was the
only safe fold; the rest pair that reference with different VCFs. Consolidating the
bare constant across 7 modules is a wider change than these follow-ups and is left
alone deliberately.
🤖 Generated with Claude Code
https://claude.ai/code/session_015VxRqNngU7Eg1wdb1aEgD6