Skip to content

Correct entropy state, motif ownership, and regional statistics - #712

Open
SuhasSrinivasan wants to merge 14 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/publish-entropy-ownership-cardinality
Open

Correct entropy state, motif ownership, and regional statistics#712
SuhasSrinivasan wants to merge 14 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/publish-entropy-ownership-cardinality

Conversation

@SuhasSrinivasan

@SuhasSrinivasan SuhasSrinivasan commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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

Summary

This PR consolidates the related entropy correctness fixes into one review surface:

  • motif-selected windows use stable half-open ownership with sufficient flanking context, including across scan stripes and BED boundaries;
  • duplicate biological anchors are unioned and incompatible combined-strand partner mappings fail before output creation;
  • entropy states use typed symbols, so valid 10+ state alphabets no longer abort;
  • regional summaries emit singleton regions and calculate medians from value-sorted copies rather than genomic encounter order.

All changes are confined to entropy state construction, motif/window ownership, and regional statistics. No threshold, CRAM, probability-sampling, DMR, or output-schema changes are included.

Severity

High — scientific correctness, statistical accuracy, and output integrity.

Affected accepted inputs can silently omit or duplicate motif anchors, report invalid window coordinates, pair incompatible anchors, omit valid singleton regions, or emit plausible but incorrect medians. Valid ten-state analyses can abort and leave incomplete output.

Bugs and corrected behavior

Case Previous behavior Corrected behavior
Motif near a window/BED/stripe boundary Anchor membership and coordinates could depend on window size or scan stripe Every anchor belongs to exactly one half-open owner; context may be inspected outside the owner without emitting outside it
Duplicate/overlapping motifs One biological anchor could be counted more than once (position, strand, base) anchors form a deterministic union
Incompatible combined partners A partner mapping could be overwritten after output creation The complete positive stream is checked for a bijection before writer construction
10 or more states Textual state ID 10 failed as a one-character value u32-backed typed symbols support the tested 9/10/12/17-state alphabets
One successful regional window The shared percentile helper rejected one value and the region row was omitted Mean/median/min/max all equal the sole entropy value
Unsorted regional values Median depended on genomic encounter order A private entropy-value copy is sorted with f32::total_cmp before the existing interpolation

The statistics fix does not mutate encounter order, window/read alignment, read counts, success/failure counts, mean, minimum, or maximum.

Implementation

  • EntropySymbol(u32) reserves explicit canonical and filtered/wildcard states; compact modification IDs remain deterministic.
  • Wildcard matching and concretization operate directly on symbol slices while preserving probability-mass behavior.
  • Search spaces carry an owner range. Stripe scans receive clipped motif context and emit only owned anchors.
  • Strand hit streams sort/deduplicate anchors and retain a fixed stripe batch plus bounded lookahead rather than contig-length hit collections.
  • Multi-motif combined-strand mode preflights partner relationships before output creation.
  • Regional medians special-case one value and otherwise sort a copy before applying the unchanged interpolation helper.

Testing

Tested exact revision: 3bfce607ffcf91217d7697c5ea1c387ad5d0eb0f
Tree: 965621babdba8c49890891a2155548d233a0c41a
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 entropy module: 24 passed after regional-statistics integration.
  • Motif geometry CLI: 4 passed.
  • State-cardinality CLI: 1 passed, internally covering 9/10/12/17 codes, forward/reverse encounter orders, and 1/4 threads.
  • Regional singleton CLI: 1 passed, exact output SHA-256 a58b12803513e46fe62df518c33db75fd7471dcb9fc7482ec3a69cf1aaabc9c3.
  • Odd/even median permutations: all 6 three-value and all 24 four-value permutations return the same correct median without mutating caller input.
  • Geometry oracle: window sizes 1/2/3/6/100 emit the same four one-base rows, SHA-256 e549001d7573f4586750a0496931e3a87bbad0672bd0aca30f6d3c7524e4c30e.
  • Stripe/thread matrix: 7/13-base stripes and 1/4 threads agree at seams, owner boundaries, duplicate anchors, and partner conflicts.
  • Cardinality output hashes are stable for K=9/10/12/17; ten equiprobable states produce entropy approximately log2(10) with 10 accepted reads.
  • Conflict/sentinel matrix: early- and later-contig partner conflicts, absent/pre-existing output, and --force all fail before creating or truncating output.
  • Dense scanner high-water fixture remains bounded by the fixed stripe plus lookahead configuration as the owner is extended.
  • Existing entropy CLI regressions passed; legacy binary/wildcard behavior is preserved.
  • git diff --check, targeted rustfmt --check, clean-worktree, ancestry, and Cargo.lock checks passed.

Bioinformatics review independently verified singleton, median, anchor ownership, partner-bijection, log2(K), wildcard-mass, and ordering invariants. Exact-head Rust review verified representation, bounded streams, preflight/failure ordering, and preservation of both source test suites. No blocker remained.

Output compatibility

  • Expected output differences are limited to incorrect motif membership/coordinates, invalid partner combinations, 10+ state failures, omitted singleton summaries, and encounter-order-dependent medians.
  • Existing entropy output schema, CLI options, threshold behavior, and unaffected binary/wildcard controls are unchanged.
  • Window-level entropy artifacts are unchanged by the regional-statistics correction.
  • Conflicting combined motifs now fail before output creation rather than leaving partial output.

Reviewer guide

  1. Review EntropySymbol and wildcard matching in methylation_entropy.rs.
  2. Review owner ranges, context clipping, global coordinate translation, anchor deduplication, and bounded hit streams in entropy/mod.rs.
  3. Review partner preflight before writer construction in entropy/subcommand.rs.
  4. Review the small singleton/value-sort statistics diff and its exhaustive permutation tests.
  5. Run cargo test -p mod_kit entropy --lib -- --test-threads=1, cargo test -p modkit --test test_entropy_geometry -- --test-threads=1, cargo test -p modkit --test test_entropy_state_cardinality -- --test-threads=1, and cargo test -p modkit --test test_entropy_region_statistics -- --test-threads=1 as focused canaries.

Non-goals

  • No threshold, record-eligibility, CRAM, output-schema, or DMR change.
  • No coverage weighting or redefinition of the regional aggregation unit.
  • No ordered floating-point reduction or fresh-process last-bit reproducibility claim.
  • No transactional rollback after a runtime writer/flush failure.
  • No claim that wildcard expansion or the complete entropy algorithm scales without bound.
  • Multi-motif combined mode deliberately performs a validation scan before its processing scan; this PR does not optimize that extra work.

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