Skip to content

Streaming GenotypeMatrix honors accessible_bed for grm/ibs (#152)#158

Open
andrewkern wants to merge 3 commits into
nsp-multiallelic-trunkfrom
fix/152-genotype-streaming-accessible-mask
Open

Streaming GenotypeMatrix honors accessible_bed for grm/ibs (#152)#158
andrewkern wants to merge 3 commits into
nsp-multiallelic-trunkfrom
fix/152-genotype-streaming-accessible-mask

Conversation

@andrewkern

Copy link
Copy Markdown
Member

Closes #152.

Problem

GenotypeMatrix.from_zarr(..., streaming=..., accessible_bed=bed) accepted accessible_bed but both streaming branches dropped it, so a StreamingGenotypeMatrix carried no accessible mask. grm/ibs then streamed over all variants while the eager path filtered to accessible sites via the mask-filtered .genotypes view -- a silent eager-vs-streaming divergence under an accessible_bed, with no error raised. This is the sibling of #151, which fixed the same drop for HaplotypeMatrix.

Fix

  • Forward accessible_bed from both GenotypeMatrix.from_zarr streaming branches into _build_streaming, resolve the mask once over the source's variant bounds, and hand it to StreamingGenotypeMatrix. iter_gpu_chunks already attaches the mask to every chunk, so per-chunk variant filtering happens for free and streaming grm/ibs match eager.
  • Factor the shared "resolve mask over source bounds" block into resolve_streaming_accessible_mask, now used by both the haplotype and genotype streaming loaders.
  • Correct the stale StreamingGenotypeMatrix docs/error text, which still claimed grm needs .materialize() -- it streams directly.

Audit

Swept the whole streaming surface for this bug class (mask honored on eager, dropped on streaming):

Conclusion: no other statistic carried a live instance of this bug. The one residual was a test-coverage gap for the load-time-mask -> streaming windowed_analysis path, closed here with a regression test.

Tests

  • Eager-vs-streaming grm/ibs parity with and without an accessible_bed, plus a guard that the mask actually changes the streaming result and a chunk-level check that each streaming chunk is restricted to accessible positions.
  • Pin the load-time mask through streaming windowed_analysis.
  • Full suite green (test_streaming_kernels, test_accessible_mask, test_relatedness, matrix/streaming loaders); ruff clean.

…m/ibs honor the mask

GenotypeMatrix.from_zarr accepted accessible_bed but its streaming
branches dropped it, so a StreamingGenotypeMatrix carried no accessible
mask. grm/ibs then streamed over all variants while the eager path
filtered to accessible sites via the mask-filtered .genotypes view -- a
silent eager-vs-streaming divergence under an accessible_bed, with no
error raised.

Forward accessible_bed from both from_zarr streaming branches into
_build_streaming, resolve the mask once over the source's variant bounds,
and hand it to StreamingGenotypeMatrix. iter_gpu_chunks already attaches
the mask to every chunk, so per-chunk variant filtering happens for free
and streaming grm/ibs match eager.

Factor the shared "resolve mask over source bounds" block into
resolve_streaming_accessible_mask, now used by both the haplotype and
genotype streaming loaders. Correct the StreamingGenotypeMatrix docs,
which still claimed grm needs .materialize() -- it streams directly.
…alysis

A load-time mask reaches windowed_analysis via a different path than an
explicit accessible_bed argument: iter_gpu_chunks attaches it to each
chunk and the per-chunk eager call honors the already-attached mask,
with no BED re-parse. The existing dispatch test only covers the
explicit-argument path. Pin the load-time path and assert the mask
actually changes the result, guarding against a silent re-drop.
…or grm/ibs

The existing grm/ibs parity tests only use streaming='always'. The
streaming='auto' branch that falls back to streaming forwards
accessible_bed through a separate _build_streaming call (source=... path).
The tiny fixture fits eagerly, so 'auto' never falls back on its own;
force the size decision to return 'streaming' with a real source and
assert the resolved mask reaches the StreamingGenotypeMatrix and grm/ibs
match eager. Guards against dropping accessible_bed from only that branch.
@andrewkern
andrewkern requested a review from nspope July 21, 2026 18:00

@nspope nspope left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good---one comment about missing test coverage.

np.testing.assert_allclose(s, e, rtol=1e-9, atol=1e-12)


class TestGrmIbsAccessibleBed:

@nspope nspope Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but doesn't have a case where there's missing data. It should be easy enough to inject missing data (-1) into the vcz --- I think that's important to cover for the parity check between streaming and eager (cover interaction between missing data (both modes) and the mask).

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.

2 participants