fix(consensus): fgbio rejection-reason parity + thread-independent duplex metrics by nh13 · Pull Request #511 · fulcrumgenomics/fgumi

nh13 · 2026-07-09T05:38:02Z

Summary

Two fgbio behavioral-parity fixes in the consensus callers and their rejection metrics (round-2 findings R2-UCC-01 and R2-MET-05 row-seeding), plus a correction to how caller-side rejection reasons map to the centralized rejection metric.

Stacked on #504 (nh/fix-metrics-fgbio-parity) — it builds on that PR's rejection-metric work. Merge #504 first; this auto-retargets to main.

What changed

Rejection-reason mapping parity. fgbio rejects unpaired/fragment reads as non_paired_reads and potential MI collisions between independent duplex molecules as potential_umi_collision (UmiConsensusCaller.scala:81,83). fgumi's caller-side RejectionReason::FragmentRead / PotentialCollision were mapped to the wrong centralized reasons (single_strand_only / duplicate_umi), so codec fragment rejections and duplex collision rejections were emitted under the wrong raw_reads_rejected_for_* keys. Added NonPairedReads / PotentialUmiCollision to the metrics RejectionReason (fgbio-exact code + description), wired them through ConsensusMetrics, and corrected to_centralized().

R2-UCC-01 — duplex fragment accounting. fgbio partitions out unpaired/fragment reads and rejects them as non_paired_reads (DuplexConsensusCaller.scala:204-205). fgumi dropped them from the R1/R2 strand groups but folded them into the input total; they are now counted as FragmentRead (→ non_paired_reads).

R2-MET-05 — always-emit duplex rejection rows. fgbio seeds each caller's usedBy* rejection reasons to zero so their rows always appear (initializeRejectCounts). Added ConsensusMetrics::to_kv_metrics_seeded + DUPLEX_SEEDED_REJECTIONS; the duplex command now seeds non_paired_reads, single_strand_only, and potential_umi_collision so those KV rows are always present.

Deliberately not ported: R2-UCC-02 (distinct cell-barcode `require`)

fgbio's callers require(barcodes.length <= 1) and throw when a source molecule carries more than one distinct cell barcode. fgumi does not replicate this, on purpose: fgumi's grouper keys on (MI, cell), so a molecule spanning multiple cell barcodes is split into per-cell groups and consensus-called per cell (lossless), where fgbio fails fast. Verified end-to-end — a same-MI/two-CB molecule yields two per-cell consensus reads (commands::simplex::tests::test_end_to_end_single_end_workflow). Because the caller therefore never sees more than one cell barcode through the CLI, a caller-level distinct-CB guard would be unreachable dead code, so it is not added. This is a deliberate, lossless architectural divergence (GRP-02 class).

Notes

Both fgbio "throws" originally in scope were confirmed against the fgbio 4.1.0 source; the distinct-CB require is real but moot for fgumi's per-cell architecture (above), while the fragment rejection is real and ported.
There are two RejectionReason enums — a caller-side one (fgumi-consensus/caller.rs) mapped via to_centralized() to the metrics one (fgumi-metrics/rejection.rs); both are updated here.

Thread-independent duplex metrics

While validating the seeding, the duplex statistics output turned out to depend on --threads: the single-threaded path wrote the wide ConsensusMetrics table while the threaded path wrote fgbio's KV format, and the threaded path reported consensus_reads_emitted from the caller stat (one per molecule) rather than the number of reads actually emitted (so a 6-read run reported 3). Both are fixed — single-threaded duplex now writes the same seeded KV format, and the threaded consensus_reads is taken from the pipeline's written-record count. Added single-vs-multi-threaded stats-equality assertions to the simplex, duplex, and codec threading-parity tests to lock the invariant. simplex (KV) and codec (wide) were already self-consistent.

Separate follow-up (not in this PR): codec writes the wide, non-KV format in both paths — self-consistent but not fgbio's KV shape.

Testing

to_kv_metrics_seeded: seeded reasons always emitted exactly once even at zero (seeded to 0), omitted from the un-seeded output at zero, and not duplicated when non-zero.
Single-vs-multi-threaded stats equality for simplex, duplex, and codec.
Updated the to_centralized() mapping-pinning tests to the corrected reasons.
Full suite: 2215 tests pass; ci-fmt and ci-lint clean.

Make fgumi's metrics `.txt` outputs readable by fgbio's `Metric.read` and match fgbio's rows, column names/order, and value tokens on a shared input. Verified end-to-end against fgbio 4.1.0's real `Metric.read` (see fixture below). - Non-finite floats (MET-02): add a shared serde helper (`float` module) that writes f64 fields as fgbio's `Infinity`/`-Infinity`/`NaN` tokens (integral values without a trailing `.0`), and apply it to every serialized f64 field across correct/shared/consensus/simplex/duplex/group. Previously only `correct.rs` was inf-safe; the rest emitted ryu `inf`/`-inf`, which fgbio's `Double.parseDouble` cannot read. - Empty-metrics header (MET-03): `write_metrics` now always emits the column header, even for a zero-row slice (the csv writer's lazy headers otherwise produce a 0-byte file that fgbio rejects with "No header found"). Route the duplex-metrics and simplex-metrics per-family/UMI/yield writes through it. - UmiGroupingMetrics (MET-04): serialize exactly fgbio's 5-column `UmiGroupingMetric` (`accepted_sam_records`, `discarded_non_pf`, `discarded_poor_alignment`, `discarded_ns_in_umi`, `discarded_umis_to_short`). The fgumi-internal summary fields (`total_records`, family aggregates) are retained in memory for logging but `#[serde(skip)]`-ed; they're derivable and the family distribution already lives in `.family_sizes.txt`. - Duplex family sizes sparse: emit one row per observed family size (matching fgbio `familySizeMetrics`) instead of a dense `1..=max` range. - Duplex UMI counts determinism: add `umi` as a secondary sort key so ties no longer fall back to nondeterministic HashMap order. - Consensus KV rejection text (MET-05, partial): match fgbio's exact rejection descriptions ("Reads has a different, and minority, set of indels"; "Only Generating One Strand of Duplex Consensus"). The caller-type-aware row-set seeding (non_paired_reads / potential_umi_collision, etc.) is deferred to the consensus PR (fgbio's usedByVanilla/Duplex/Codec seeding + R2-UCC-01), as it requires new RejectionReason variants wired into the duplex/codec callers. - format_float doc (MET-01): correct the doc-comment that falsely claimed it matches fgbio; numeric equality (not byte format) is the metric contract. CorrectUmis zero-count seeding (#498 item 5) already reproduces as fixed on main: both correct execution paths pre-seed a row per expected UMI plus the all-N sentinel. No change needed; kept as-is. fgbio-oracle evidence (reports/fgbio-parity-fixtures/met-02-03-04-fgbio-read.sh): fgbio 4.1.0 `Metric.read[UmiGroupingMetric]` parses fgumi's 5-column file and its header-only empty file (0 rows, no error); `Metric.read[UmiCorrectionMetrics]` parses `representation=Infinity` — while the old ryu `inf` token is rejected. CI: cargo ci-fmt / ci-lint clean; full suite 2215 passed.

…_reads/potential_umi_collision fgbio's UmiConsensusCaller rejects unpaired/fragment reads as `non_paired_reads` and potential MI collisions between independent duplex molecules as `potential_umi_collision` (UmiConsensusCaller.scala:81,83). fgumi's caller-side `RejectionReason::FragmentRead` and `PotentialCollision` were mapped by `to_centralized()` to the wrong centralized reasons (`single_strand_only` and `duplicate_umi`), so codec fragment rejections and duplex collision rejections were emitted under the wrong `raw_reads_rejected_for_*` metric keys. Add `NonPairedReads` and `PotentialUmiCollision` to the metrics `RejectionReason` (fgbio-exact code + description), wire them through `ConsensusMetrics` (count/add/emit), and correct the `to_centralized()` mapping so codec now emits `non_paired_reads` and duplex now emits `potential_umi_collision`. Also adds `RejectionReason::used_by_duplex()` (mirroring fgbio's `usedByDuplex` flags) for the follow-on per-caller rejection-row seeding (R2-MET-05). Part of R2-UCC-01. Branch stacked on #504 (nh/fix-metrics-fgbio-parity).

coderabbitai · 2026-07-09T05:38:10Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 44 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d3a8b91e-42ae-4505-85c1-44d781c80edf

📥 Commits

Reviewing files that changed from the base of the PR and between b0fd52c and 104d226.

📒 Files selected for processing (7)

crates/fgumi-consensus/src/caller.rs
crates/fgumi-consensus/src/duplex_caller.rs
crates/fgumi-metrics/src/consensus.rs
crates/fgumi-metrics/src/rejection.rs
src/lib/commands/codec.rs
src/lib/commands/duplex.rs
src/lib/commands/simplex.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch nh/fix-consensus-rejection-parity

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-07-09T05:40:33Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.09%. Comparing base (b0fd52c) to head (104d226).

Additional details and impacted files

@@                       Coverage Diff                       @@
##           nh/fix-metrics-fgbio-parity     #511      +/-   ##
===============================================================
- Coverage                        91.12%   91.09%   -0.04%     
===============================================================
  Files                               78       78              
  Lines                            51535    51562      +27     
===============================================================
+ Hits                             46960    46969       +9     
- Misses                            4575     4593      +18

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ex rejection rows Two fgbio parity fixes in the duplex caller and its rejection metrics. R2-UCC-01 — duplex fragment accounting. fgbio partitions unpaired/fragment reads out and rejects them as `non_paired_reads` (DuplexConsensusCaller.scala:204-205). fgumi dropped them from the R1/R2 strand groups but folded them into the input total; they are now counted as `FragmentRead` (→ `non_paired_reads`). R2-MET-05 — always-emit duplex rejection rows. fgbio seeds each caller's `usedBy*` rejection reasons to zero so their rows always appear (initializeRejectCounts). Add `ConsensusMetrics::to_kv_metrics_seeded` + `DUPLEX_SEEDED_REJECTIONS`; the duplex command seeds `non_paired_reads`, `single_strand_only`, and `potential_umi_collision` so those KV rows are always present. R2-UCC-02 (fgbio's distinct cell-barcode `require`) is deliberately NOT ported: fgumi's grouper keys on `(MI, cell)`, so a molecule spanning multiple cell barcodes is split into per-cell groups and consensus-called per cell (lossless), whereas fgbio's `require(barcodes.length <= 1)` fails fast. The caller therefore never sees more than one cell barcode via the CLI (verified: a same-MI/two-CB molecule yields two per-cell consensus reads), so a caller-level guard would be unreachable dead code. This is a deliberate, lossless architectural divergence. Tests: `to_kv_metrics_seeded` always-emits-at-zero and is not duplicated when non-zero. Full suite passes; ci-fmt/ci-lint clean. Branch stacked on #504.

…ity) The duplex statistics output depended on `--threads`: - Format flip: the single-threaded path wrote the wide `ConsensusMetrics` table (`write_tsv([metrics])`) while the threaded path wrote fgbio's KV vertical format (`to_kv_metrics_seeded`). Single-threaded duplex now writes the same seeded KV format, so the seeded `usedByDuplex` rejection rows (R2-MET-05) are emitted regardless of thread count. - `consensus_reads_emitted` mismatch: the threaded path reported this from the caller stat (which counts consensus events — one per molecule) instead of the number of reads actually written (R1+R2 per molecule), so a run emitting 6 consensus reads reported 3. The threaded path now sets `consensus_reads` from the pipeline's written-record count, matching the single-threaded path and the physical output. Adds single-vs-multi-threaded stats-equality assertions to the simplex, duplex, and codec threading-parity tests to lock the invariant that metrics output must not depend on thread count. simplex (KV) and codec (wide) were already self-consistent; duplex is fixed here. Note: codec still writes the wide (non-KV) format in both paths — self-consistent but not fgbio's KV format. Converting codec metrics to KV is a separate parity item, not addressed here.

nh13 · 2026-07-09T20:40:01Z

Superseded by #508.

The rejection-reason parity here (seed duplex KV rows; map fragment/collision rejections to non_paired_reads/potential_umi_collision) is implemented in #508 via the to_kv_metrics(ConsensusCallerKind) API. This PR's unique contribution — making duplex's single-threaded stats match the multi-threaded seeded KV format (thread-independent metrics) — has been folded into #508: duplex execute() now emits to_kv_metrics(ConsensusCallerKind::Duplex), so both paths (and both simplex paths) write the same seeded key-value format. Closing as absorbed.

The `codec` command wrote its statistics as the wide `ConsensusMetrics` table (`write_tsv([metrics])`) in both the single- and multi-threaded paths, unlike `simplex` and `duplex` which emit fgbio's vertical key/value/description format (`ConsensusKvMetric`). The wide format is not readable by fgbio's `Metric.read` and does not carry the `raw_reads_rejected_for_*` rejection rows in a fgbio-parseable shape. Convert both codec paths to `to_kv_metrics_seeded`, seeding the `usedByCodec` rejection reasons fgumi models (`non_paired_reads`, `single_strand_only`, `potential_umi_collision`) via a new `CODEC_SEEDED_REJECTIONS`. Codec is a duplex-style caller, so it shares the duplex-seeded set. The metrics output was already thread-consistent for codec (both paths use the same caller stats); this keeps that and adds a KV-format assertion to the codec threading-parity test. Note: fgbio additionally seeds codec-specific rejection reasons (`r1_r2_overlap_too_short`, `indel_error_between_strands`, `high_duplex_disagreement`, `clip_overlap_failed`, `not_primary_fr_pair`) that fgumi does not yet model as distinct `RejectionReason`s — a separate follow-up. Stacked on #511 (uses its `to_kv_metrics_seeded` + rejection-reason work).

) The `codec` command wrote its statistics as the wide `ConsensusMetrics` table (`write_tsv([metrics])`) in both the single- and multi-threaded paths, unlike `simplex` and `duplex` which emit fgbio's vertical key/value/description format (`ConsensusKvMetric`). The wide format is not readable by fgbio's `Metric.read` and does not carry the `raw_reads_rejected_for_*` rejection rows in a fgbio-parseable shape. Convert both codec paths to `to_kv_metrics_seeded`, seeding the `usedByCodec` rejection reasons fgumi models (`non_paired_reads`, `single_strand_only`, `potential_umi_collision`) via a new `CODEC_SEEDED_REJECTIONS`. Codec is a duplex-style caller, so it shares the duplex-seeded set. The metrics output was already thread-consistent for codec (both paths use the same caller stats); this keeps that and adds a KV-format assertion to the codec threading-parity test. Note: fgbio additionally seeds codec-specific rejection reasons (`r1_r2_overlap_too_short`, `indel_error_between_strands`, `high_duplex_disagreement`, `clip_overlap_failed`, `not_primary_fr_pair`) that fgumi does not yet model as distinct `RejectionReason`s — a separate follow-up. Stacked on #511 (uses its `to_kv_metrics_seeded` + rejection-reason work).

nh13 added 2 commits July 8, 2026 17:53

nh13 temporarily deployed to github-actions July 9, 2026 05:38 — with GitHub Actions Inactive

nh13 force-pushed the nh/fix-consensus-rejection-parity branch from e732f43 to d04e3d9 Compare July 9, 2026 05:56

nh13 temporarily deployed to github-actions July 9, 2026 05:56 — with GitHub Actions Inactive

nh13 temporarily deployed to github-actions July 9, 2026 06:07 — with GitHub Actions Inactive

nh13 changed the title ~~fix(consensus): fgbio rejection-reason parity (distinct-CB guard, duplex fragments, seeded rows)~~ fix(consensus): fgbio rejection-reason parity + thread-independent duplex metrics Jul 9, 2026

nh13 mentioned this pull request Jul 9, 2026

fix(codec): emit fgbio KV metrics format (thread-consistent, seeded rows) #513

Merged

nh13 force-pushed the nh/fix-metrics-fgbio-parity branch from b0fd52c to c5e61ce Compare July 9, 2026 15:10

nh13 mentioned this pull request Jul 9, 2026

fix(codec): model codec-specific fgbio rejection reasons + seed KV rows #516

Merged

nh13 closed this Jul 9, 2026

Navigation Menu

Repository navigation

fix(consensus): fgbio rejection-reason parity + thread-independent duplex metrics - #511

fix(consensus): fgbio rejection-reason parity + thread-independent duplex metrics#511
nh13 wants to merge 4 commits into
nh/fix-metrics-fgbio-parityfrom
nh/fix-consensus-rejection-parity

nh13 commented Jul 9, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

Review limit reached

Uh oh!

codecov Bot commented Jul 9, 2026 •

edited

Loading

Uh oh!

nh13 commented Jul 9, 2026

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

nh13 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Deliberately not ported: R2-UCC-02 (distinct cell-barcode require)

Notes

Thread-independent duplex metrics

Testing

Uh oh!

coderabbitai Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

codecov Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nh13 commented Jul 9, 2026

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

nh13 commented Jul 9, 2026 •

edited

Loading

Deliberately not ported: R2-UCC-02 (distinct cell-barcode `require`)

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

codecov Bot commented Jul 9, 2026 •

edited

Loading