Skip to content

Preserve complete and ordered DMR segmentation - #703

Closed
SuhasSrinivasan wants to merge 4 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/preserve-dmr-contig-order
Closed

Preserve complete and ordered DMR segmentation#703
SuhasSrinivasan wants to merge 4 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/preserve-dmr-contig-order

Conversation

@SuhasSrinivasan

Copy link
Copy Markdown
Contributor

Fixes #686.
Fixes #695.

Summary

  • Return one HMM state for every successfully scored DMR site, including the final site in each nonempty chunk.
  • Preserve the lexical contig order established by the DMR scheduler before scores reach the site writer or stateful segmenter.
  • Add independent exhaustive-path, singleton/boundary, real chromosome-20, deterministic contig-order, interval-geometry, and thread regressions.

Severity

Severity: High — scientific correctness, output completeness, and reproducibility

Rationale: The decoder silently omitted one scored site per HMM chunk, and unordered contig collection could close and reopen a contig at internal batch boundaries. Both alter segment boundaries, counts, aggregates, effects, and state continuity on accepted input.

Root cause

The Viterbi backtrace replaced the final emitted state with its predecessor and encoded an N-1 path/position assumption. Separately, process_batch_of_positions collected outer FxHashMap entries directly, replacing the scheduler's lexical order with hash iteration before the stateful receiver.

Implementation

  • Start backtrace at the final emitted state, traverse only emission predecessors, reverse the complete path, and require path/position cardinality equality.
  • Convert complete paths to half-open regions, including singleton final positions.
  • Sort unique contig tuples lexically at the final batch handoff without cloning or reordering scores within a contig.
  • Keep the four commits as two tests/fix pairs for direct review.

Preserved behavior

  • Emissions, transitions, priors, distance projection, per-site scoring, floating-point order, state vocabulary, and output schemas are unchanged.
  • Complexity remains O(N) for decoding; contig handoff adds O(C log C) comparisons for normally small C.

Non-goals

  • No single-site denominator/replicate-statistics change.
  • No automatic-coverage, writer-lifecycle, batching-policy, or natural-contig-order redesign.

Behavior before and after

Case Before After Oracle
One emission Zero states/regions One state and [pos,pos+1) One state per emission
Chromosome-20 fixture 17,271 site rows but segment total 17,270 Both totals 17,271 Every successful site covered once
Cross-contig batch Contig could leave/re-enter and split one segment Lexical one-block-per-contig stream Continuous 15-site bb segment
Interval/thread matrix Geometry could change segments Byte-identical across interval 10/3 and threads 1/1, 4/2 Internal batching invariance

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: 88c703a / 5f778a963a3b7d79e79bba81fb5685e29ef6a453; clean tracked worktree; four linear tests/fix commits on upstream 5cecc3f.
  • Parent-red HMM regression: singleton and multi-score path cardinality failed; real CLI segment total was 17,270 versus 17,271 sites.
  • Parent-red contig regressions: deterministic no-op helper failed lexical order, and the three-contig CLI fixture produced contig re-entry/split segmentation.
  • cargo test --offline --locked -p mod_kit hmm -- --test-threads=1: 3 passed, 0 failed.
  • cargo test --offline --locked -p mod_kit dmr::single_site -- --test-threads=1: 4 passed, 0 failed.
  • cargo test --offline --locked -p modkit --test test_dmr -- --test-threads=1: 4 passed, 0 failed.
  • Exact-head cargo test --offline --locked --workspace --all-targets -- --test-threads=1: 187 active tests passed, 14 declared ignored, 0 failed.
  • Chromosome-20 fixed output: 17,271 represented sites; final end 10,804,378; site SHA-256 f9c0e056…; complete segment SHA-256 aa3b3782….
  • Three-contig output conserves 6 aa, 15 bb, and 6 cc sites; repaired segments are aa [0,6), bb [0,15), cc [0,6); interval/thread artifacts are byte-identical.
  • Added BGZF/index/reference fixtures passed integrity checks.
  • git diff --check upstream/master...HEAD passed; worktree clean.
  • Repository-wide stable cargo fmt --all -- --check reports unchanged upstream formatting plus nightly-only settings; scoped changed-file checks had previously passed with only those setting warnings.

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 the affected path.
  • Performance/RSS benchmarks were not run; only source-level complexity was reviewed.

Scientific validation

  • Every successful site supplies one emission, receives one state, and belongs to exactly one segment.
  • Segment num_sites reconciles with represented site rows.
  • A contig cannot be artificially closed/reopened by hash iteration inside a completed batch.
  • Site and segment bytes are invariant across tested interval and thread configurations.

Output and compatibility

  • Affected segment ends, boundaries, counts, aggregates, effects, scores, row counts, and contig order intentionally change.
  • Nonsegmented schema, CLI, half-open BED convention, and unaffected single-contig scoring remain unchanged.
  • Both linked defects are combined because contig ordering feeds the same stateful HMM whose cardinality contract is corrected first; the commit pairs remain independently reviewable.

Reviewer guide

  1. Review the exhaustive decoder oracle and compact backtrace in hmm.rs.
  2. Review path_to_region_labels and cardinality tests.
  3. Review the lexical handoff helper and three-contig fixture.
  4. Rerun the three focused commands above.

Checklist

  • Both issues contain reproducible observed and expected behavior.
  • The four commits are two focused tests/fix pairs.
  • Parent-red and fix-green evidence is recorded.
  • All material tests and omissions are listed.
  • Site/segment counts, coordinates, and determinism are checked.
  • Diff hygiene passed; unrelated format findings are disclosed.
  • Only minimal public synthetic fixtures are included.

@SuhasSrinivasan

Copy link
Copy Markdown
Contributor Author

This completed segmentation fix is now consolidated into #704 with the related DMR denominator, replicate, and positive-coverage corrections. The exact combined head received independent Rust and Bioinformatics review and passed the full serial workspace gate (208 passed, 14 ignored, 0 failed). Closing this duplicate review surface so DMR science can be reviewed in one PR.

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