Skip to content

Reconcile and stabilize summary output - #699

Open
SuhasSrinivasan wants to merge 2 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-summary-output-order
Open

Reconcile and stabilize summary output#699
SuhasSrinivasan wants to merge 2 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-summary-output-order

Conversation

@SuhasSrinivasan

Copy link
Copy Markdown
Contributor

Fixes #672.

Summary

  • Emit summary bases and modification codes in deterministic biological/code order.
  • Include categories that occur only among filtered calls so table and TSV category rows reconcile with pass/fail/all totals.
  • Report sampling fractions as percentages in the sampling log.
  • Add opposite-insertion-order, filtered-only letter/ChEBI, zero-denominator, CLI, and repeated-process regressions.

Severity

Severity: High — scientific reporting and reproducibility

Rationale: Filtered-only categories could disappear even though they contributed to totals, making the report internally inconsistent and hiding biological states. Hash-dependent row order and incorrect log units were Medium issues separately.

Root cause

Writers iterated hash-backed maps directly and constructed rows primarily from passing states. Filtered-only states therefore had no row, while map iteration determined output order. The log rendered a unit fraction with a percent sign without multiplying by 100.

Implementation

  • Build the output state union from passing, filtered, and observed states.
  • Sort canonical bases A/C/G/T, place canonical rows first, then sort modification codes by stable letter-code and numeric-ChEBI order.
  • Guard zero category denominators and convert sampling fractions to percent only for logging.

Preserved behavior

  • Aggregate sampling decisions and pass/fail/all counts are unchanged.
  • TSV/table schemas and threshold calculations are unchanged.

Non-goals

  • No global ordering contract for unrelated commands.
  • No probability sampling or threshold algorithm change.

Behavior before and after

Case Before After Oracle
Filtered-only modification code Category row omitted Row has pass 0 and exact fail/all counts Category sums reconcile with totals
Opposite insertion orders Row order could differ Byte-identical output Stable A/C/G/T and code order
Fraction 0.1 log Displayed as 0.1% Displayed as 10% Sampling decision unchanged
Empty category denominator NaN-like presentation possible Numeric zero Finite report

Testing

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

  • Revision/tree: ba73cfc / a997d5633902e5833c901bb85d3ff545f3e81bcf; clean tracked worktree.
  • Parent-red CLI and insertion-order tests reproduced missing filtered-only rows, hash-dependent order, and 0.1% logging.
  • cargo test --offline --locked -p mod_kit summary -- --test-threads=1: 6 passed, 0 failed.
  • cargo test --offline --locked -p modkit --test test_summary -- --test-threads=1: 4 passed, 0 failed.
  • cargo test --offline --locked --workspace --all-targets -- --test-threads=1: 185 active tests passed, 14 declared ignored, 0 failed.
  • Focused writer/log records: five writer ordering/reconciliation tests and one sampling-log test passed.
  • Real CLI table/TSV comparisons across two processes and threads 1/2/4 were byte-identical after the fix; the 0.1/seed-42 control sampled the same ten reads before and after.
  • 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 settings; no unrelated rewrite was made.
  • Test setup note: a first concurrent full-suite attempt collided on shared temporary BAM filenames and failed test_adjust_canonical. The exact revision passed the complete suite when rerun serially.

Tests not performed

  • cargo clippy was not run.
  • Performance/RSS benchmarking was not run; the change sorts small report key sets and does not alter sampling work.

Scientific validation

  • Sum of emitted category pass/fail/all values reconciles with the report totals.
  • Filtered-only canonical, letter-code, and numeric-ChEBI states are represented exactly once.
  • Output order is independent of map insertion and Rayon/process scheduling.
  • Sampling-log conversion does not alter the sampled population.

Output and compatibility

  • Affected reports intentionally gain previously omitted rows and stable ordering.
  • Counts, thresholds, schemas, and sampling decisions are unchanged.
  • Downstream byte comparisons become reproducible.

Reviewer guide

  1. Review the state-union and sorting helpers in writers.rs.
  2. Review the filtered-only/zero-denominator tests and the one-line log conversion.
  3. Rerun the two focused commands above.

Checklist

  • The issue contains reproducible observed and expected behavior.
  • The change is limited to summary reporting and logging.
  • Parent-red and fix-green evidence is recorded.
  • All material tests and setup failures are listed.
  • Count reconciliation and deterministic output are 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.

summary output can be nondeterministic and omit filtered-only categories

1 participant