Skip to content

Apply edge filters consistently during probability sampling - #696

Open
SuhasSrinivasan wants to merge 3 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-probability-edge-filter-consistency
Open

Apply edge filters consistently during probability sampling#696
SuhasSrinivasan wants to merge 3 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-probability-edge-filter-consistency

Conversation

@SuhasSrinivasan

Copy link
Copy Markdown
Contributor

Fixes #667.

Summary

  • Apply normal and inverted edge filters to the same molecular read positions in indexed and streaming probability collection.
  • Use checked edge arithmetic so overlong trims produce an empty population rather than underflowing or selecting unrelated calls.
  • Add forward/reverse, mapped/unmapped, asymmetric, inverted, and overlong-filter regressions that reconcile the threshold population with extract output.

Severity

Severity: High — scientific correctness and reproducibility

Rationale: The previous indexed and streaming paths could estimate automatic thresholds from different calls, or from read interiors while output retained read ends. That changes downstream pass/fail classifications solely because of index/routing details.

Root cause

Probability collection had several separate filter implementations. Indexed handlers did not consistently apply the requested trim, reverse-strand coordinates were not always converted to molecular orientation, and the streaming inverted branch selected the complement of the output population. Unsigned subtraction also made overlong filters unsafe.

Implementation

  • Centralize the checked molecular-position predicate and use it in all indexed handlers and both streaming branches.
  • Convert aligned query positions to the same forward molecular coordinate before applying asymmetric filters.
  • Treat filters whose trimmed lengths consume the read as selecting no calls.

Preserved behavior

  • No-filter probability histograms and extraction output are unchanged.
  • Threshold percentile calculation, modification-code handling, and CLI syntax are unchanged.

Non-goals

Behavior before and after

Case Before After Oracle
Inverted filter on the nine-call fixture Threshold population used five interior q220 calls while output retained the four ends Threshold population and output use the same four end calls Median quality 150; the q1 call passes
Indexed forward/reverse asymmetric filter Missing or misoriented filtering Exact molecular-position parity with streaming collection Indexed and streaming histograms match
Overlong filter Unsafe arithmetic or inconsistent population Empty population without underflow No retained calls
No filter Existing histogram Unchanged Exact artifact identity

Testing

Environment: macOS 26.6 arm64; rustc/cargo 1.90.0; installed modkit 0.6.4 reference; ignored test-only Cargo.lock SHA-256 78876ab4a98da30caad167744d1c8a0875c27edad7b0b3ad5f8a1891f78604ea resolving hts-sys 2.2.0. Cargo.lock is not in the diff.

  • Revision/tree: e043ada / 7064d426cf6f86c6d6f63fa1bbccbcf455fdeb9d; clean tracked worktree.
  • Parent-red: installed 0.6.4 and upstream 5cecc3f reproduce ignored indexed trims, reverse misorientation, and the inverted threshold/output mismatch.
  • cargo test --offline --locked -p mod_kit edge_filter -- --test-threads=1: 5 passed, 0 failed.
  • cargo test --offline --locked -p modkit --test test_extract edge_filter -- --test-threads=1: 3 passed, 0 failed.
  • cargo test --offline --locked --workspace --all-targets -- --test-threads=1: 186 active tests passed, 14 declared ignored, 0 failed.
  • Forward/reverse, mapped/unmapped, normal/inverted, asymmetric/overlong, stranded-position, no-filter, and pass-only CLI matrices passed; q149/q150/q1 outcomes matched the independent oracle.
  • git diff --check upstream/master...HEAD passed; the worktree remained clean.
  • Repository-wide stable cargo fmt --all -- --check reports unchanged upstream formatting plus nightly-only rustfmt settings, so it was not used to rewrite unrelated files.
  • Test setup note: an initial attempt to run three worktree-wide suites concurrently collided on repository tests' shared global temporary BAM names. This branch's run completed successfully; the other affected suites were rerun serially.

Tests not performed

  • cargo clippy was not run.
  • The 1-GB direct-RNA slice was not used because the exact synthetic records provide the required strand, edge, and threshold-population oracle.

Scientific validation

  • The probability population contains exactly the calls eligible for the requested output-side edge filter.
  • Reverse records use molecular rather than reference orientation.
  • Indexed and streaming paths agree exactly for equivalent eligible records.
  • Repeated and thread-varied controls retain deterministic histograms.

Output and compatibility

  • Affected automatic thresholds and pass-only rows intentionally change to use the requested population.
  • No-filter controls and output schemas are unchanged.
  • The branch is independent of the open dependency-resolution PR Pin hts-sys to bindings compatible with rust-htslib 0.46 #666; author tests used hts-sys 2.2.0.

Reviewer guide

  1. Review the shared predicate and the four indexed handlers in modkit-core/src/sample_probs/mod.rs.
  2. Review the streaming normal/inverted branches and checked overlong-filter behavior.
  3. Rerun the two focused commands above.

Checklist

  • The issue contains reproducible observed and expected behavior.
  • The change is limited to the linked issue.
  • Parent-red and fix-green evidence is recorded.
  • All material tests and setup failures are listed.
  • Scientific/output compatibility is checked.
  • Diff hygiene passed; unrelated format findings are disclosed.
  • No private data, generated lockfile, or unrelated change is included.

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.

Probability sampling applies --edge-filter inconsistently across execution paths

1 participant