Skip to content

Exclude unmapped calls from CpG summaries - #650

Open
SuhasSrinivasan wants to merge 1 commit into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-summary-cpg-unmapped
Open

Exclude unmapped calls from CpG summaries#650
SuhasSrinivasan wants to merge 1 commit into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-summary-cpg-unmapped

Conversation

@SuhasSrinivasan

@SuhasSrinivasan SuhasSrinivasan commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #649.

Summary

  • Exclude unmapped records from indexed summary --cpg results, matching the documented --motif CG 0 behavior.
  • Preserve the mapped CpG histogram and Summary schema.
  • Add an end-to-end CLI regression containing a valid unmapped MM/ML/MN record.

Severity

Severity: High — scientific correctness

Rationale: The shorthand can silently include an ineligible unmapped population and alter read totals and modification statistics while the command exits successfully.

Root cause

The indexed Summary path correctly constructs a CG motif for --cpg, but the later guard that skips unmapped collection checked only mapped_only, matched_only, and an explicit motif. Omitting the CpG shorthand allowed all unmapped calls to be merged into the mapped reference-CpG histogram.

Implementation

Treat --cpg as a resolved motif restriction in the unmapped-collection guard. The regression copies the 10 mapped test records and appends an unmapped CCCC record with MM:Z:C+m?,0,0,0,0;, four ML values of 255, and MN:i:4, then compares normalized CLI output with mapped-only and explicit-motif controls.

Preserved behavior

  • Mapped CpG motif matching and thresholds are unchanged.
  • Summary keys, category definitions, and output schema are unchanged.
  • Explicit --motif CG 0 behavior remains unchanged.

Non-goals

  • Does not change mapped motif lookup.
  • Does not change fractional/fixed-count sampling or automatic thresholds.
  • Does not redefine modification classes or other unmapped-record modes.

Behavior before and after

Case Before After Expected oracle
Mixed 10-mapped/10-unmapped issue fixture under --cpg 20 reads, 186 C calls 10 reads, 77 C calls Equal to --motif CG 0 and mapped-only control
Focused test with one appended unmapped record total_reads_used=11 total_reads_used=10 Unmapped record contributes no Summary category

Testing

Test environment

  • Revision tested: a99f411a8dacfd4bf705fc9adb068219bc7bafc7
  • Tree tested and worktree state: 0b532f8ee0fd65de5e489f32485792209182112f; clean at final verification
  • Platform: macOS on Apple silicon
  • Reference binary: installed modkit 0.6.4
  • External tool: samtools 1.23.1 for fixture construction
  • Exact Rust version was not retained in the original test log.
  • Dependency resolution: existing workspace manifests; no dependency or lockfile change in this PR
Test layer Exact command, fixture, or matrix Result and evidence
Core: parent-red regression Final focused test applied to base 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c Failed as expected: total_reads_used=11 instead of 10
Core: focused regression cargo test -p modkit --test test_summary test_summary_cpg_excludes_unmapped_calls -- --exact Passed
Core: affected integration tests cargo test -p modkit --test test_summary 5 passed, 0 failed
Neighboring pileup integration cargo test -p modkit --test test_pileup -- --test-threads=1 16 passed, 10 ignored, 0 failed
Core: applicable full workspace gate cargo test --workspace --quiet -- --test-threads=1 Passed; aggregate count was not retained in the original log
Installed-versus-fixed comparison Issue fixture under --cpg, explicit --motif CG 0, and mapped-only control 20 / 186 becomes 10 / 77 and matches both controls
Core: formatting/diff checks rustfmt --check --edition 2021 modkit-core/src/modbam_util/subcommands.rs modkit/tests/test_summary.rs; git diff --check Passed
Disclosed initial test failure Initial default-parallel package run; exact invocation was not retained Failed because existing test_pileup_no_filt and test_pileup_with_header share /tmp/test_pileup_nofilt.bed; serial affected and workspace gates pass, and this PR does not touch pileup

Tests not performed

  • No performance/RSS benchmark was run; the production change adds one boolean term outside the per-call hot path.
  • No large real-data or CRAM matrix was run; the exact mixed indexed-BAM fixture directly exercises the affected branch.

Scientific validation

  • Population invariant: Reference-CpG summaries contain only records assignable to a reference motif.
  • Equivalence invariant: --cpg equals explicit --motif CG 0 after normalizing key order.
  • Count invariant: The unmapped record changes neither read totals nor canonical/modified categories.
  • Independent oracle: The mapped-only fixture and explicit motif command independently produce 10 reads and 77 C calls.

Output and compatibility

  • Only the incorrect unmapped contribution is removed.
  • Summary schema and mapped-record behavior are unchanged.
  • The documented shorthand becomes behaviorally consistent with its explicit form.

Reviewer guide

  1. Review the mixed mapped/unmapped CLI regression and its two independent controls.
  2. Review the one-condition change in the indexed Summary unmapped guard.
  3. Rerun cargo test -p modkit --test test_summary test_summary_cpg_excludes_unmapped_calls -- --exact.
  4. Rerun cargo test -p modkit --test test_summary as the affected integration canary.

Checklist

  • Linked issue contains reproducible observed and expected behavior.
  • Change is limited to the linked issue's scope.
  • Regression is red on the parent and green on this revision.
  • All tests performed, including the unrelated initial failure, are disclosed.
  • Unrun applicable tests are disclosed.
  • Scientific population/count invariants and compatibility are checked.
  • Changed-file formatting and diff hygiene pass.

@SuhasSrinivasan
SuhasSrinivasan marked this pull request as ready for review August 5, 2026 05:56
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.

summary --cpg includes unmapped modification calls

1 participant