Skip to content

Make indexed sampling and threshold caps deterministic - #705

Open
SuhasSrinivasan wants to merge 5 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/u11-deterministic-seeded-sampling
Open

Make indexed sampling and threshold caps deterministic#705
SuhasSrinivasan wants to merge 5 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/u11-deterministic-seeded-sampling

Conversation

@SuhasSrinivasan

@SuhasSrinivasan SuhasSrinivasan commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review status: Author-reviewed and ready for ONT review.

Fixes #688.
Fixes #689.

Summary

  • Make seeded indexed fractional sampling depend on a stable alignment identity rather than worker/interval scheduling.
  • Honor explicit seeds in the legacy mapped indexed path, including sparse retained intervals and reads spanning interval boundaries.
  • Validate fractional sampling bounds before workers start.
  • Reduce explicit-canonical threshold caps to the global per-base maximum across every sampled read.

Severity

Severity: High — scientific reproducibility and correctness

Rationale: A fixed input/fraction/seed could select a different population when threads or interval geometry changed. Independently, an arbitrary hash/fold survivor could replace the true explicit-canonical cap, changing thresholds and downstream pass/fail calls.

Root cause

Sampling decisions consumed schedule-local RNG state, and the legacy indexed path did not consistently apply the user seed or carry ownership across sparse retained intervals. Explicit-canonical caps overwrote rather than maximized per-read values inside a parallel fold.

Implementation

  • Derive Bernoulli decisions from a versioned stable alignment identity plus the user seed.
  • Validate finite sampling fractions in [0,1] centrally for every consumer.
  • Filter/annotate retained intervals before parallel work and carry the previous retained end across super-batches so spanning reads are handled exactly once.
  • Use the global per-base maximum for explicit-canonical probability caps.

Preserved behavior

  • Fractions 0 and 1 remain exact.
  • Count sampling, unindexed sampling, and indexed-unmapped policy remain outside this change.
  • Percentile computation and sampled call probabilities are unchanged once the population/cap is fixed.

Non-goals

  • No promise of stable identity across a future sampler-version change without an explicit version transition.
  • No CRAM reference plumbing, empty-threshold, zero-target, or edge-filter change.

Behavior before and after

Case Before After Oracle
Same seed, threads 1/2/3/8 Sampled population could differ Exact same alignment decisions Stable identity hash
Sparse indexed intervals Gap-starting/spanning reads lost or duplicated Each eligible read handled once Interval-geometry identity
Invalid fraction Reached workers/runtime Immediate CLI validation error Finite inclusive [0,1]
Explicit canonical values 0.71 and 0.99 Arbitrary fold survivor could be 0.71 Cap is 0.99 Global maximum

Testing

Environment: macOS 26.6 arm64; rustc/cargo 1.90.0; ignored test-only Cargo.lock SHA-256 49c08c4c51b6f4320726551146d971fa9ef2183d40c3f6c631b2005965e242c0 resolving hts-sys 2.2.0. Cargo.lock is not in the diff.

  • Revision/tree: 615eb8b / d46e516c0093fd0f1370739c1498f6928191f9b4; clean tracked worktree.
  • Parent-red matrices reproduced schedule-dependent sampled maps/thresholds, ignored legacy seeds, sparse interval ownership errors, invalid-fraction late handling, and arbitrary 0.71 instead of the global 0.99 cap.
  • cargo test --offline --locked -p mod_kit deterministic_sampler -- --test-threads=1: 7 passed, 0 failed.
  • cargo test --offline --locked -p mod_kit explicit_canonical_probability_is_global_max_across_reads -- --test-threads=1: 1 passed, 0 failed.
  • cargo test --offline --locked -p modkit --test test_sample_probs_determinism -- --test-threads=1: 5 passed, 0 failed.
  • cargo test --offline --locked -p modkit --test test_legacy_threshold_sampling_determinism -- --test-threads=1: 4 passed, 0 failed.
  • cargo test --offline --locked -p modkit --test test_sampling_fraction -- --test-threads=1: 3 passed, 0 failed.
  • Exact-head cargo test --offline --locked --workspace --all-targets -- --test-threads=1: 209 active tests passed, 14 declared ignored, 0 failed.
  • Matrices covered repeated processes, workers 1/2/3/8, interval sizes/orders, sparse gaps, spanning reads, different seeds, fractions 0/1 and invalid negative/>1/NaN/infinite values, and exact sampled-map/threshold output.
  • git diff --check upstream/master...HEAD passed; worktree clean.
  • Repository-wide stable cargo fmt --all -- --check reports unchanged upstream formatting plus nightly-only settings; no unrelated rewrite was made.
  • Test setup note: an initial exact-name filter selected zero canonical-cap tests; the corrected non-exact filter selected and passed the intended single regression.

Tests not performed

  • cargo clippy was not run.
  • Broad performance/RSS benchmarks were not run.
  • CRAM reference, empty automatic-threshold, and zero-target fixes are separate issues and are not included.

Scientific validation

  • One seed/input/fraction identifies one sampled alignment set independent of scheduling and interval geometry.
  • Eligible spanning reads are sampled exactly once.
  • Explicit-canonical threshold caps equal the global per-base maximum across sampled reads.
  • Different seeds demonstrably diverge while repeated identical seeds remain exact.

Output and compatibility

Reviewer guide

  1. Review stable identity and deterministic sampler construction.
  2. Review retained-interval ownership in reads_sampler.
  3. Review the one-line maximum reduction and centralized fraction validation.
  4. Rerun the five focused commands above.

Checklist

  • Both issues contain reproducible observed and expected behavior.
  • The five commits are limited to deterministic fraction sampling and canonical caps.
  • Parent-red and fix-green evidence is recorded.
  • All material tests and setup notes are listed.
  • Population, threshold, and deterministic controls are checked.
  • Diff hygiene passed; unrelated format findings are disclosed.
  • No private data, generated lockfile, or unrelated change is included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant