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

Root cause and impact

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.

Filtering must happen per bedMethyl record before aggregation. Applying the threshold after summing offsets would allow multiple individually under-covered loci to combine and pass.

Regression

The test uses two records from the existing indexed bedMethyl resource that localize to the same output bin:

  • valid coverage 1, modified count 1
  • valid coverage 23, modified count 2

At --min-coverage 1, the exact aggregate remains 24 valid / 3 modified / 12.5%. At --min-coverage 3, the first record is excluded and the exact result is 23 valid / 2 modified / 8.695652%.

Validation

  • confirmed the finalized regression fails on the parent revision: threshold 3 still reports 24 / 3 / 12.5 instead of 23 / 2 / 8.695652
  • cargo test -p modkit --test test_localize test_localize_min_coverage_filters_before_aggregation -- --exact
  • cargo test -p modkit --test test_localize (2 passed)
  • cargo test --workspace --quiet -- --test-threads=1
  • rustfmt --check --edition 2021 modkit-core/src/localise/subcommand.rs modkit-core/src/localise/util.rs modkit/tests/test_localize.rs
  • git diff --check
  • installed-versus-patched CLI comparison on the issue fixture confirms thresholds 1 and 3 are identical before the fix and differ exactly as expected after it

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

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