Skip to content

Correct Localize coordinates, coverage, validation, and charts - #700

Open
SuhasSrinivasan wants to merge 9 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-localize-reporting-and-regions
Open

Correct Localize coordinates, coverage, validation, and charts#700
SuhasSrinivasan wants to merge 9 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-localize-reporting-and-regions

Conversation

@SuhasSrinivasan

@SuhasSrinivasan SuhasSrinivasan commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review status: Author-reviewed and ready for ONT review.

Summary

This PR consolidates Localize's related eligibility, coordinate, regional-I/O, and chart corrections:

  • --min-coverage filters individual bedMethyl rows before strand filtering and offset aggregation, with inclusive valid_coverage >= min_coverage semantics;
  • feature windows use symmetric half-open geometry around the original anchor, and offsets use one reference-axis sign convention;
  • complete region files are validated before output creation, and regional query/decode failures propagate from both localize and stats;
  • charts use percent units, support sparse/singleton offsets, and do not create or truncate output for empty data.

These changes define one Localize input-to-profile contract. Existing table columns and CLI options are unchanged.

Severity

High — scientific eligibility, coordinates, counts, and output integrity.

The accepted coverage option previously had no effect; the coordinate defect changed which observations entered profiles and assigned retained observations to incorrect bins. Invalid region rows or query errors could also yield incomplete successful output.

Bugs and corrected behavior

Case Previous behavior Corrected behavior
--min-coverage 3 with coverage-1 and coverage-23 records in one bin Both rows aggregated to 24 valid / 3 modified / 12.5% Coverage-1 row is removed first: 23 / 2 / 8.695652%
Interior anchor and window w Shifted/asymmetric membership and reversed offset Query [anchor-w, anchor+w+1) and emit position-anchor
Edge-clipped feature Anchor could be reconstructed from the clipped interval Original feature anchor remains the offset origin
Mixed valid/invalid BED Partial successful aggregate possible Contextual nonzero failure before output creation
Corrupt regional query Error could be silently skipped Contextual command failure; no incomplete success
TSV value 50 / sparse chart Chart could plot 0.5, fail, or leave an empty file Chart plots 50 and supports one datum

Implementation

  • Pass min_cov into FocusRegion::into_localized_mod_counts and filter by valid_coverage >= min_coverage before strand selection/folding.
  • Preserve the original feature anchor while clipping only the query interval.
  • Parse and validate complete BED3+ input with physical-line context before opening output; columns 7+ remain opaque.
  • Use contextual try_fold/try_reduce propagation for regional work while retaining missing-contig skipping.
  • Plot numeric percent values, pad singleton x bounds, show one-datum markers, and reject empty chart data before file creation.

Testing

Tested exact revision: a290d201e189c6c4ea375fddea2036bd7601b88c
Tree: c13548ff4087cdd07e5e3548edca43ed4aa2ace3
Platform/toolchain: macOS arm64; rustc/cargo 1.90.0; rust-htslib 0.46.0 and hts-sys 2.2.0.

  • Full serial workspace gate: 212 passed, 14 ignored, 0 failed.
  • Focused Localize units: 23 passed.
  • Localize CLI integrations: 9 passed.
  • Stats CLI integrations: 2 passed.
  • Adapted minimum-coverage oracle uses corrected --window 0 geometry and offset 0: threshold 1 gives C, 0, 24, 3, 12.5; threshold 3 gives C, 0, 23, 2, 8.695652.
  • Parser matrix covers BED3–BED6+, blank/comment rows, spaces in names, opaque extra columns, delimiter/field/score/strand/bounds failures, exact line diagnostics, and absent/sentinel output preservation.
  • Coordinate matrix covers window 0/2, coordinate zero, chromosome edges, plus/minus/unstranded features, and exact reference-axis bins.
  • Valid localize/stats outputs are byte-identical at threads 1 and 4; regional failure fixtures do not create absent output or overwrite sentinels.
  • Chart matrix covers percent identity, one datum, multiple series at one offset, empty data, and multi-offset controls; table bytes remain unchanged by chart-only corrections.
  • git diff --check, targeted rustfmt --check, conflict-marker scan, clean-worktree, ancestry, source-oracle union, and Cargo.lock checks passed.

Rust and Bioinformatics reviews independently verified inclusive pre-aggregation coverage filtering, anchor/window geometry, reference-axis offsets, failure propagation, chart units, and preservation of all source tests. No blocker remained.

Output compatibility

  • Affected low-coverage rows, window membership, offsets, counts, and chart values intentionally change; historical affected profiles should be regenerated.
  • Valid schemas, CLI syntax, missing-contig behavior, and threshold-boundary inclusion are unchanged.
  • Invalid/regional-failure inputs now terminate nonzero without mutating output.

Reviewer guide

  1. Review minimum-coverage plumbing and its adapted window-zero oracle.
  2. Review feature anchor/window construction and offset sign in subcommand.rs and util.rs.
  3. Review parser-before-output ordering and regional try_fold/try_reduce paths.
  4. Review percent/sparse/empty chart regressions.
  5. Run cargo test -p mod_kit localise -- --test-threads=1, cargo test -p modkit --test test_localize -- --test-threads=1, and cargo test -p modkit --test test_stats -- --test-threads=1 as focused canaries.

Non-goals

  • No feature-oriented strand reversal; offsets use the reference axis.
  • No Localize batch-size policy change.
  • No shared writer-finalization or general pipeline-cancellation redesign.
  • No performance claim; coverage filtering adds one scalar comparison per fetched bedMethyl record.

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.

1 participant