fix(svar2): normalize contig names on the read path (#336) - #340
Conversation
`Svar2Haps` carries both the dataset's contig spelling (`ds_contigs`, from the BED) and the store's (`store_contigs`, from the source VCF) and never reconciled them. All seven read-bound decode call sites passed the dataset spelling straight into the Rust kernels, which look contigs up in the store's own keyspace. Since the write path already normalizes, a dataset over a differently-named .svar2 store -- e.g. a UCSC-named `chr1` BED over an Ensembl-named `1` store, a supported arrangement -- built successfully and was then unreadable with `ValueError: contig chr1 not in store`. This affected every Svar2Haps read (haplotypes, diffs, tracks, variants), not just window mode. Resolve once per contig in `__post_init__` via genoray's `ContigNormalizer` -- the mechanism already used for this throughout the codebase, including by the SVAR2 write path -- and hand the kernels the store's spelling via `_store_contig(ci)`. Resolution is eager but reporting stays lazy, so a dataset that lists a contig the store lacks and never queries it keeps working. The new error names both spellings, which the Rust-side message could not. Closes mcvickerlab#336 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
VerificationRun against the dependency floor this branch's base actually requires ( 1. The new test against unpatched 2. The same test with the fix: 3. Existing SVAR2 suite with the fix — no regressions: Note on the issue's suggested patchThe issue proposed Hence |
Fixes #336.
Problem
Svar2Hapscarries two contig spellings and never reconciles them:A dataset's contigs come from its BED (
_write._prep_bed:natsorted(bed["chrom"].unique())), while the store carries whatever spelling its source VCF used. The two are allowed to differ — a UCSC-namedchr1BED over an Ensembl-named1store is a supported arrangement, andgvl.writereconciles the two at write time.But all seven read-bound decode call sites passed the dataset spelling straight into the Rust kernels, which look contigs up in the store's keyspace:
So the write path normalized and the read path did not. A dataset in this arrangement builds successfully and is then unreadable:
This is not specific to window mode — it affects every
Svar2Hapsread (haplotypes, variants, diffs, tracks). The equivalent SVAR1 path normalizes correctly, which is why this went unnoticed: SVAR2 is newer, and no test covered a build-then-read cycle across differing contig conventions.Fix
Resolve once per contig at open rather than once per query, and hand the kernels the store's spelling:
All seven sites become
self._store_contig(ci).ContigNormalizeris the mechanism already used for this everywhere else in the codebase (_write.py,_open.py,_reference.py,_table.py,_dummy.py), including by the SVAR2 write path — so this makes the read path agree with the write path rather than introducing a new convention. It also picks up mito-alias handling (chrM↔MT) for free.Two deliberate choices
Resolution is eager, reporting is lazy. The table is built at open (it is per-contig, not per-query, so this costs nothing on the read path), but an unmappable contig raises only when that contig is actually queried. Raising at open would regress datasets that list a contig the store lacks and simply never query it — that works today, and this keeps working.
The error names both spellings. The previous message was
contig chr1 not in storefrom the Rust side, which does not tell you what the store does call its contigs. The new one does, which is the whole diagnostic difficulty of this class of bug.The
_store_contigindirection also means a future call site cannot reintroduce the bug by reaching fords_contigs— there is now one obvious way to get a contig name for the store.Tests
New
tests/dataset/test_svar2_contig_naming.py:test_svar2_read_normalizes_contig_naming— builds a.svar2store named1, writes a dataset from achr1BED (asserting the two spellings genuinely differ on disk), then reads it. This is the issue's reproducer and fails onmainwithValueError: contig chr1 not in store.test_svar2_unmapped_contig_names_both_spellings— a genuinely unmappable contig still raises, names both spellings, and does not raise at construction (pinning the lazy-reporting choice above).No behavior change when the dataset and store already agree:
ContigNormalizermaps every name to itself in that case, which the existing SVAR2 suite covers.🤖 Generated with Claude Code