Skip to content

Correct coverage and replicate handling in single-site DMR - #704

Open
SuhasSrinivasan wants to merge 6 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-single-site-dmr-coverage-and-replicates
Open

Correct coverage and replicate handling in single-site DMR#704
SuhasSrinivasan wants to merge 6 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-single-site-dmr-coverage-and-replicates

Conversation

@SuhasSrinivasan

Copy link
Copy Markdown
Contributor

Fixes #687.
Fixes #693.
Fixes #694.

Summary

  • Reject raw or coverage-capped zero denominators before single-site DMR statistics emit NaN or reach singular beta-density calculations.
  • Preserve configured sample identity for matched-replicate statistics and suppress both replicate fields when any configured pair is incomplete.
  • Exclude absent, zero-coverage, and coverage-filtered observations from pooled/balanced statistics and representation percentages while preserving observed canonical 0/n calls.
  • Add exact numerical, pairing, filtering, ordering, thread, and failure-path regressions.

Severity

Severity: Medium — scientific correctness, output validity, and reliability

Rationale: Accepted input could produce NaN or abort, impute no-observation replicates as canonical, or assign plausible matched statistics to the wrong pairs. Each changes scientific output silently within single-site DMR scoring.

Root cause

Count/cap boundaries allowed total zero, organizer data discarded sample IDs before pairing, and balanced aggregation converted undefined zero-total fractions into canonical pseudo-counts. These adjacent paths used inconsistent meanings for “no observation.”

Implementation

  • Reject zero totals and resolved zero caps, check replicate-cap multiplication, and keep CLI prior versus posterior beta-domain validation distinct.
  • Carry sample IDs through organization, consume configured CLI order, and emit replicate fields only when every configured pair has positive coverage.
  • Collapse only positive-coverage observations and calculate represented-sample percentage floors with exact integer arithmetic.
  • Retain six linear commits alternating tests and fixes for the three linked issues.

Preserved behavior

  • Positive A=10/10 modified versus B=0/10 modified retains MAP p-value 0.0000006230948043897833 and effect 1.
  • Observed positive-coverage canonical 0/n remains a real observation.
  • Valid pooled/group rows remain available when optional matched fields are incomplete.
  • CLI/output columns and the “-” sentinel grammar are unchanged.

Non-goals

  • No change to the MAP estimand, posterior update, quadrature, automatic-cap percentile population, segmentation, region-level DMR, or writer lifecycle.
  • No clamping zero caps to one and no partial replicate-vector grammar.

Behavior before and after

Case Before After Oracle
Raw/capped zero denominator NaN success or process abort Recoverable failed site or early configuration error No scientific NaN row
Three configured pairs Cross-paired effects [0.5,-1,0.5] Command-order effects [1,-0.5,-0.5] Independent one-pair commands
Any incomplete pair Remaining samples compacted/cross-paired Both replicate fields “-” No fabricated statistic
Zero-coverage replicate Balanced effect 0.5; representation 100/100 Effect 1; representation 50/100 Same as physically absent/filtered control
Observed canonical 0/10 Real observation Unchanged Exact positive-coverage control

Testing

Environment: macOS 26.6 arm64; rustc/cargo 1.90.0; installed modkit 0.6.4 reference; samtools/bgzip/tabix 1.23.1; ignored test-only Cargo.lock SHA-256 78876ab4a98da30caad167744d1c8a0875c27edad7b0b3ad5f8a1891f78604ea resolving hts-sys 2.2.0. Cargo.lock is not in the diff.

  • Revision/tree: f7a4331 / 5dd3ae4b5b7b4acc78a3cf78fc59c63cb6de2401; clean tracked worktree; six tests/fix commits directly on upstream 5cecc3f.
  • Parent-red matrices reproduced NaN/status-134 zero paths, explicit/automatic zero-cap failures, cross-pairing, incomplete-pair fabrication, and zero-row pseudo-count bias.
  • cargo test --offline --locked -p mod_kit dmr:: -- --test-threads=1: 26 passed, 0 failed.
  • cargo test --offline --locked -p modkit --test test_dmr -- --test-threads=1: 11 passed, 0 failed.
  • cargo test --release --offline --locked -p mod_kit represented_sample_percentages_use_exact_integer_floor -- --test-threads=1: 1 passed, 0 failed.
  • Exact-head cargo test --offline --locked --workspace --all-targets -- --test-threads=1: 200 active tests passed, 14 declared ignored, 0 failed.
  • Numerical controls covered positive cap 10/10, prior boundaries, complete and incomplete pair matrices, zero/absent/filtered equivalence, 1/1 versus 4/2 threads, and no accepted repaired NaN.
  • All 24 added fixture/index/reference artifacts, including seven BGZF inputs, passed integrity checks.
  • git diff --check upstream/master...HEAD and scoped changed-file rustfmt checks passed; worktree clean.
  • Test setup notes: an initial command used nonexistent package name modkit-core, then was corrected to mod_kit. Recomposition also exposed one test-helper call that needed unwrap after the constructor became fallible; all cited gates ran after that correction.
  • Repository-wide stable cargo fmt --all -- --check reports unchanged upstream formatting plus nightly-only settings; no unrelated rewrite was made.

Tests not performed

  • cargo clippy was not run.
  • The direct-RNA BAM slice was not used because dmr pair consumes indexed bedMethyl/FASTA inputs for this path.
  • Performance/RSS benchmarks were not run; source review found no new sort, synchronization, or normal-path heap-allocation class.

Scientific validation

  • Only positive-total observations enter pooled/balanced aggregates and representation numerators.
  • Configured sample count remains the representation denominator; observed canonical 0/n remains eligible.
  • Pairing follows immutable CLI identity, not hash/vector order.
  • No accepted count fraction has a zero denominator or singular paired beta parameter.

Output and compatibility

  • Invalid zero cases now fail normally; affected pooled/balanced/representation and matched-replicate fields intentionally change.
  • Positive numerical controls, schemas, options, and valid group rows are retained.
  • The three fixes share the single-site observation/pairing boundary and are presented as separately reviewable tests/fix commit pairs.

Reviewer guide

  1. Review denominator/cap checks in beta_diff.rs.
  2. Review sample identity organization in dmr/tabix.rs and matched construction in single_site.rs.
  3. Review positive-only collapse and exact percentage arithmetic.
  4. Rerun the two focused commands above.

Checklist

  • All three issues contain reproducible observed and expected behavior.
  • The six commits are three focused tests/fix pairs.
  • Parent-red and fix-green evidence is recorded.
  • All material tests and setup notes are listed.
  • Numerical, count, pairing, and deterministic controls are checked.
  • Diff hygiene passed; unrelated format findings are disclosed.
  • Only minimal public synthetic fixtures are 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