Skip to content

Apply localize minimum coverage before aggregation - #652

Open
SuhasSrinivasan wants to merge 1 commit into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-localize-min-coverage
Open

Apply localize minimum coverage before aggregation#652
SuhasSrinivasan wants to merge 1 commit into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-localize-min-coverage

Conversation

@SuhasSrinivasan

@SuhasSrinivasan SuhasSrinivasan commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #651.

Summary

  • Pass --min-coverage into localize record processing.
  • Retain each bedMethyl record only when valid_coverage >= min_coverage, before offset aggregation.
  • Add an exact end-to-end CLI regression covering below, equal-to, and above-threshold records.

Severity

Severity: High — scientific correctness

Rationale: The accepted option previously had no effect, silently allowing excluded low-coverage records to change localized scientific counts and percentages.

Root cause

EntryLocalize::run assigned and logged self.min_coverage but did not pass it into GenomeRegion::into_localized_mod_counts. The aggregation therefore folded every fetched bedMethyl row into n_valid and n_mod regardless of the requested threshold.

Implementation

Pass the minimum coverage into localized counting and filter each bedMethyl record before aggregation using the inclusive predicate valid_coverage >= min_coverage. The CLI regression uses two records that aggregate into the same output bin, with (valid coverage, modified count) values (1,1) and (23,2).

Preserved behavior

  • Records equal to the threshold remain included.
  • Aggregation, output ordering, and schema are unchanged after eligibility filtering.
  • Default behavior is unchanged for records meeting the default threshold.

Non-goals

  • Does not change localize anchor, window, or offset conventions.
  • Does not define or implement --batch-size behavior.
  • Does not change chart rendering or regional worker-error handling.

Behavior before and after

Case Before After Expected oracle
--min-coverage 1 24 valid / 3 modified / 12.5% Same Both records meet the inclusive threshold
--min-coverage 3 24 valid / 3 modified / 12.5% 23 valid / 2 modified / 8.695652% Coverage-1 record excluded before aggregation

Testing

Test environment

  • Revision tested: 57c61c561a44addafe04f9b44dfd6de01cff5f95
  • Tree tested and worktree state: e476b271f2bba441f9933c1894a16bf7b5d51096; clean at final verification
  • Platform: macOS on Apple silicon
  • Reference binary: installed modkit 0.6.4
  • Exact Rust and tabix versions were 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 CLI regression applied to base 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c Failed as expected: threshold 3 still produced 24 / 3 / 12.5 instead of 23 / 2 / 8.695652
Core: focused regression cargo test -p modkit --test test_localize test_localize_min_coverage_filters_before_aggregation -- --exact Passed
Core: affected integration tests cargo test -p modkit --test test_localize 2 passed, 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 at thresholds 1 and 3 Installed outputs are identical; fixed output changes only threshold 3 to the exact 23/2/8.695652 oracle
Core: formatting/diff checks rustfmt --check --edition 2021 modkit-core/src/localise/subcommand.rs modkit-core/src/localise/util.rs modkit/tests/test_localize.rs; git diff --check Passed

The serial workspace mode avoids an existing unrelated collision between pileup integration tests that share a temporary output filename.

Tests not performed

  • No performance/RSS benchmark was run; the change adds one scalar comparison per fetched bedMethyl record.
  • No large real-data regression was run; the two-record fixture distinguishes per-record filtering from an incorrect post-aggregation filter exactly.

Scientific validation

  • Eligibility invariant: A record contributes if and only if valid_coverage >= min_coverage.
  • Aggregation invariant: Filtering occurs before records sharing an output offset are combined.
  • Count conservation: Threshold 1 sums (1,1)+(23,2) to (24,3); threshold 3 retains (23,2) only.
  • Independent oracle: Direct inspection with tabix supplies the two source coverage/count pairs.

Output and compatibility

  • Only rows below the requested threshold are removed from aggregates.
  • TSV schema, offset ordering, and CLI syntax are unchanged.
  • Threshold-boundary records remain included.

Reviewer guide

  1. Review the two-record CLI fixture and its pre-aggregation oracle.
  2. Review the threshold plumbing into GenomeRegion::into_localized_mod_counts.
  3. Rerun cargo test -p modkit --test test_localize test_localize_min_coverage_filters_before_aggregation -- --exact.
  4. Rerun cargo test -p modkit --test test_localize 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 are listed with their results.
  • Unrun applicable tests are disclosed.
  • Scientific eligibility/count invariants and compatibility are checked.
  • Changed-file formatting and diff hygiene pass.

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.

localize --min-coverage does not filter bedMethyl records

1 participant