Skip to content

fix(svar2): normalize contig names on the read path (#336) - #340

Merged
d-laub merged 1 commit into
mcvickerlab:mainfrom
d-laub:fix/336-svar2-read-contig-norm
Jul 31, 2026
Merged

fix(svar2): normalize contig names on the read path (#336)#340
d-laub merged 1 commit into
mcvickerlab:mainfrom
d-laub:fix/336-svar2-read-contig-norm

Conversation

@d-laub

@d-laub d-laub commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes #336.

Problem

Svar2Haps carries two contig spellings and never reconciles them:

store_contigs: list[str]   # the .svar2 store's own names, e.g. ["1", "2", ...]
ds_contigs:    list[str]   # the dataset's names, e.g. ["chr1", "chr2", ...]

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-named chr1 BED over an Ensembl-named 1 store is a supported arrangement, and gvl.write reconciles 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:

465, 512, 617, 721, 858, 950, 1095:   self.ds_contigs[ci],

So the write path normalized and the read path did not. A dataset in this arrangement builds successfully and is then unreadable:

ValueError: contig chr1 not in store

This is not specific to window mode — it affects every Svar2Haps read (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:

_ds_to_store: list[str | None] = field(init=False)

def __post_init__(self):
    ...
    self._ds_to_store = ContigNormalizer(self.store_contigs).norm(self.ds_contigs)

def _store_contig(self, ci: int) -> str:
    contig = self._ds_to_store[ci]
    if contig is None:
        raise ValueError(
            f"Dataset contig {self.ds_contigs[ci]!r} has no equivalent in the"
            f" .svar2 store, whose contigs are {self.store_contigs}."
        )
    return contig

All seven sites become self._store_contig(ci).

ContigNormalizer is 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 (chrMMT) 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 store from 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_contig indirection also means a future call site cannot reintroduce the bug by reaching for ds_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 .svar2 store named 1, writes a dataset from a chr1 BED (asserting the two spellings genuinely differ on disk), then reads it. This is the issue's reproducer and fails on main with ValueError: 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: ContigNormalizer maps every name to itself in that case, which the existing SVAR2 suite covers.

🤖 Generated with Claude Code

`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>
@d-laub

d-laub commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Verification

Run against the dependency floor this branch's base actually requires (genoray 3.3.1, seqpro 0.22.0) — worth stating explicitly, because on older genoray the write path itself raises KeyError: 'chr1' and the scenario is unreachable.

1. The new test against unpatched main — fails with the issue's exact error:

FAILED tests/dataset/test_svar2_contig_naming.py::test_svar2_read_normalizes_contig_naming
  - ValueError: contig chr1 not in store
FAILED tests/dataset/test_svar2_contig_naming.py::test_svar2_unmapped_contig_names_both_spellings
  - AttributeError: 'Svar2Haps' object has no attribute '_ds_to_store'
2 failed in 3.10s

2. The same test with the fix:

tests/dataset/test_svar2_contig_naming.py::test_svar2_read_normalizes_contig_naming PASSED
tests/dataset/test_svar2_contig_naming.py::test_svar2_unmapped_contig_names_both_spellings PASSED
2 passed in 1.11s

3. Existing SVAR2 suite with the fix — no regressions:

tests/dataset/test_svar2_readbound_variants.py  tests/dataset/test_svar2_readbound_haps.py
tests/dataset/test_svar2_readbound_diffs.py     tests/dataset/test_svar2_readbound_tracks.py
tests/dataset/test_svar2_dataset.py             tests/dataset/test_write_svar2.py
tests/dataset/test_svar2_fields_read.py         tests/unit/dataset/test_svar2_store.py

62 passed in 55.56s

Note on the issue's suggested patch

The issue proposed self.store._resolve_contig(...). That method belongs to genoray's SparseVar2, not to the store field here — Svar2Store is this crate's own pyo3 class (src/svar2/store.rs), a plain HashMap<String, ContigReader> with no normalization. It also proposed assigning a new attribute in __post_init__, which @dataclass(slots=True) rejects.

Hence ContigNormalizer(self.store_contigs) behind a declared field(init=False) instead.

@d-laub
d-laub merged commit ef4ab74 into mcvickerlab:main Jul 31, 2026
8 checks passed
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.

Svar2Haps read path does not normalize contig names, so a chr-named dataset over an Ensembl-named .svar2 store writes fine but cannot be read

1 participant