Repository navigation
fix(metrics): align TSV metric output with fgbio Metric format (#498) - #504
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMetric TSV output now uses shared f64 serialization, matches fgbio’s grouping schema, preserves headers for empty outputs, emits sparse duplex family sizes, and orders tied duplex UMI counts deterministically. Rejection descriptions match fgbio wording. ChangesMetrics output compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant MetricsCommand
participant MetricsWriter
participant FloatSerde
participant TSVFile
MetricsCommand->>MetricsWriter: write metric collection
MetricsWriter->>FloatSerde: serialize metric f64 fields
FloatSerde->>TSVFile: emit fgbio-compatible values
MetricsWriter->>TSVFile: preserve header for empty collections
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #504 +/- ##
==========================================
+ Coverage 92.60% 92.61% +0.01%
==========================================
Files 165 166 +1
Lines 99636 99803 +167
==========================================
+ Hits 92265 92431 +166
- Misses 7371 7372 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…2-MET-05, R2-UCC-01)
Bring the duplex caller's KV consensus statistics in line with fgbio's per-caller
rejection-row model.
R2-UCC-01 (S4): fgbio's DuplexConsensusCaller partitions unpaired/fragment reads
out and rejects them as NonPairedReads
(`val (pairs, frags) = recs.partition(_.paired); rejectRecords(frags, NonPairedReads)`);
fgumi had no such reason and folded fragments into the strand groups, where they
were ignored by strand grouping but mis-accounted. Add a NonPairedReads rejection
reason (metrics + ConsensusMetrics field), map the caller-side FragmentRead reason
to it (it previously mis-mapped to SameStrandOnly), and partition + reject
fragments in DuplexConsensusCaller::consensus_reads.
R2-MET-05 (S4): fgbio seeds and always emits per-caller rejection rows via
`initializeRejectCounts` (`UmiConsensusCaller.scala:223-224`) — the duplex caller
emits `raw_reads_rejected_for_{non_paired_reads,single_strand_only,potential_umi_collision}`
even at zero, in addition to the four vanilla rows. fgumi emitted only the four
vanilla rows and put the duplex reasons in a non-zero-only "optional" set. Replace
the fixed core/optional split in ConsensusMetrics::to_kv_metrics with per-caller
seeding driven by a new ConsensusCallerKind {Vanilla, Duplex, Codec}: seeded rows
are always emitted (in fgbio order), and fgumi's finer-grained reasons are emitted
only when non-zero. Re-key the collision reason from `duplicate_umi` to fgbio's
`potential_umi_collision` and align the three newly seeded rows' descriptions with
fgbio.
(This is the row-set half of R2-MET-05; #504 owns the remaining description-text
parity. Codec does not currently emit KV stats, so codec seeding is defined but
unexercised; simplex already matched fgbio's four vanilla rows.)
Verified fgbio<->fgumi end to end
(reports/fgbio-parity-fixtures/r2-met-05-ucc-01-duplex-rejection-rows.sh):
- R2-MET-05: fgbio duplex emits 7 rejection rows; fgumi BEFORE emitted 4; fgumi
AFTER emits the same 7 rows (keys match fgbio exactly).
- R2-UCC-01: with one fragment read, fgbio reports non_paired_reads=1; fgumi
BEFORE emitted no such row; fgumi AFTER reports non_paired_reads=1.
…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.
b0fd52c to
c5e61ce
Compare
…2-MET-05, R2-UCC-01)
Bring the duplex caller's KV consensus statistics in line with fgbio's per-caller
rejection-row model.
R2-UCC-01 (S4): fgbio's DuplexConsensusCaller partitions unpaired/fragment reads
out and rejects them as NonPairedReads
(`val (pairs, frags) = recs.partition(_.paired); rejectRecords(frags, NonPairedReads)`);
fgumi had no such reason and folded fragments into the strand groups, where they
were ignored by strand grouping but mis-accounted. Add a NonPairedReads rejection
reason (metrics + ConsensusMetrics field), map the caller-side FragmentRead reason
to it (it previously mis-mapped to SameStrandOnly), and partition + reject
fragments in DuplexConsensusCaller::consensus_reads.
R2-MET-05 (S4): fgbio seeds and always emits per-caller rejection rows via
`initializeRejectCounts` (`UmiConsensusCaller.scala:223-224`) — the duplex caller
emits `raw_reads_rejected_for_{non_paired_reads,single_strand_only,potential_umi_collision}`
even at zero, in addition to the four vanilla rows. fgumi emitted only the four
vanilla rows and put the duplex reasons in a non-zero-only "optional" set. Replace
the fixed core/optional split in ConsensusMetrics::to_kv_metrics with per-caller
seeding driven by a new ConsensusCallerKind {Vanilla, Duplex, Codec}: seeded rows
are always emitted (in fgbio order), and fgumi's finer-grained reasons are emitted
only when non-zero. Re-key the collision reason from `duplicate_umi` to fgbio's
`potential_umi_collision` and align the three newly seeded rows' descriptions with
fgbio.
(This is the row-set half of R2-MET-05; #504 owns the remaining description-text
parity. Codec does not currently emit KV stats, so codec seeding is defined but
unexercised; simplex already matched fgbio's four vanilla rows.)
Verified fgbio<->fgumi end to end
(reports/fgbio-parity-fixtures/r2-met-05-ucc-01-duplex-rejection-rows.sh):
- R2-MET-05: fgbio duplex emits 7 rejection rows; fgumi BEFORE emitted 4; fgumi
AFTER emits the same 7 rows (keys match fgbio exactly).
- R2-UCC-01: with one fragment read, fgbio reports non_paired_reads=1; fgumi
BEFORE emitted no such row; fgumi AFTER reports non_paired_reads=1.
…2-MET-05, R2-UCC-01)
Bring the duplex caller's KV consensus statistics in line with fgbio's per-caller
rejection-row model.
R2-UCC-01 (S4): fgbio's DuplexConsensusCaller partitions unpaired/fragment reads
out and rejects them as NonPairedReads
(`val (pairs, frags) = recs.partition(_.paired); rejectRecords(frags, NonPairedReads)`);
fgumi had no such reason and folded fragments into the strand groups, where they
were ignored by strand grouping but mis-accounted. Add a NonPairedReads rejection
reason (metrics + ConsensusMetrics field), map the caller-side FragmentRead reason
to it (it previously mis-mapped to SameStrandOnly), and partition + reject
fragments in DuplexConsensusCaller::consensus_reads.
R2-MET-05 (S4): fgbio seeds and always emits per-caller rejection rows via
`initializeRejectCounts` (`UmiConsensusCaller.scala:223-224`) — the duplex caller
emits `raw_reads_rejected_for_{non_paired_reads,single_strand_only,potential_umi_collision}`
even at zero, in addition to the four vanilla rows. fgumi emitted only the four
vanilla rows and put the duplex reasons in a non-zero-only "optional" set. Replace
the fixed core/optional split in ConsensusMetrics::to_kv_metrics with per-caller
seeding driven by a new ConsensusCallerKind {Vanilla, Duplex, Codec}: seeded rows
are always emitted (in fgbio order), and fgumi's finer-grained reasons are emitted
only when non-zero. Re-key the collision reason from `duplicate_umi` to fgbio's
`potential_umi_collision` and align the three newly seeded rows' descriptions with
fgbio.
(This is the row-set half of R2-MET-05; #504 owns the remaining description-text
parity. Codec does not currently emit KV stats, so codec seeding is defined but
unexercised; simplex already matched fgbio's four vanilla rows.)
Verified fgbio<->fgumi end to end
(reports/fgbio-parity-fixtures/r2-met-05-ucc-01-duplex-rejection-rows.sh):
- R2-MET-05: fgbio duplex emits 7 rejection rows; fgumi BEFORE emitted 4; fgumi
AFTER emits the same 7 rows (keys match fgbio exactly).
- R2-UCC-01: with one fragment read, fgbio reports non_paired_reads=1; fgumi
BEFORE emitted no such row; fgumi AFTER reports non_paired_reads=1.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-metrics/src/duplex.rs`:
- Around line 330-353: Add a fixture-backed golden test for
DuplexMetrics::family_size_metrics comparing the emitted sparse rows and
fractions against an fgbio-generated family_sizes.txt baseline. Extend the
existing tests near test_family_size_metrics_are_sparse and
test_family_size_metrics_union_disjoint_cs_ss_ds_keys, loading representative
fixture input and asserting the generated output matches the golden file
exactly.
In `@crates/fgumi-metrics/src/rejection.rs`:
- Around line 59-60: Update the test_to_kv_metrics_* tests to assert exact
description strings for raw_reads_rejected_for_minority_alignment and
raw_reads_rejected_for_single_strand_only, matching the descriptions returned by
MinorityAlignment and SameStrandOnly and preserving fgbio parity.
In `@src/lib/commands/duplex_metrics.rs`:
- Around line 224-231: Reuse the UMI metrics already computed before the duplex
block instead of calling main_collector.umi_metrics() again. Update the
duplex_umi_metrics calculation in the duplex_umi_counts branch to pass the
existing metrics value into main_collector.duplex_umi_metrics, preserving the
current output and writing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ba74c13-0704-4429-804e-dc1c82b1e62b
📒 Files selected for processing (16)
crates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/correct.rscrates/fgumi-metrics/src/duplex.rscrates/fgumi-metrics/src/float.rscrates/fgumi-metrics/src/group.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/rejection.rscrates/fgumi-metrics/src/shared.rscrates/fgumi-metrics/src/simplex.rscrates/fgumi-metrics/src/writer.rssrc/lib/commands/clip.rssrc/lib/commands/duplex_metrics.rssrc/lib/commands/group.rssrc/lib/commands/simplex_metrics.rssrc/lib/metrics/mod.rstests/integration/test_group_command.rs
c5e61ce to
dd984c8
Compare
…2-MET-05, R2-UCC-01)
Bring the duplex caller's KV consensus statistics in line with fgbio's per-caller
rejection-row model.
R2-UCC-01 (S4): fgbio's DuplexConsensusCaller partitions unpaired/fragment reads
out and rejects them as NonPairedReads
(`val (pairs, frags) = recs.partition(_.paired); rejectRecords(frags, NonPairedReads)`);
fgumi had no such reason and folded fragments into the strand groups, where they
were ignored by strand grouping but mis-accounted. Add a NonPairedReads rejection
reason (metrics + ConsensusMetrics field), map the caller-side FragmentRead reason
to it (it previously mis-mapped to SameStrandOnly), and partition + reject
fragments in DuplexConsensusCaller::consensus_reads.
R2-MET-05 (S4): fgbio seeds and always emits per-caller rejection rows via
`initializeRejectCounts` (`UmiConsensusCaller.scala:223-224`) — the duplex caller
emits `raw_reads_rejected_for_{non_paired_reads,single_strand_only,potential_umi_collision}`
even at zero, in addition to the four vanilla rows. fgumi emitted only the four
vanilla rows and put the duplex reasons in a non-zero-only "optional" set. Replace
the fixed core/optional split in ConsensusMetrics::to_kv_metrics with per-caller
seeding driven by a new ConsensusCallerKind {Vanilla, Duplex, Codec}: seeded rows
are always emitted (in fgbio order), and fgumi's finer-grained reasons are emitted
only when non-zero. Re-key the collision reason from `duplicate_umi` to fgbio's
`potential_umi_collision` and align the three newly seeded rows' descriptions with
fgbio.
(This is the row-set half of R2-MET-05; #504 owns the remaining description-text
parity. Codec does not currently emit KV stats, so codec seeding is defined but
unexercised; simplex already matched fgbio's four vanilla rows.)
Verified fgbio<->fgumi end to end
(reports/fgbio-parity-fixtures/r2-met-05-ucc-01-duplex-rejection-rows.sh):
- R2-MET-05: fgbio duplex emits 7 rejection rows; fgumi BEFORE emitted 4; fgumi
AFTER emits the same 7 rows (keys match fgbio exactly).
- R2-UCC-01: with one fragment read, fgbio reports non_paired_reads=1; fgumi
BEFORE emitted no such row; fgumi AFTER reports non_paired_reads=1.
…2-MET-05, R2-UCC-01)
Bring the duplex caller's KV consensus statistics in line with fgbio's per-caller
rejection-row model.
R2-UCC-01 (S4): fgbio's DuplexConsensusCaller partitions unpaired/fragment reads
out and rejects them as NonPairedReads
(`val (pairs, frags) = recs.partition(_.paired); rejectRecords(frags, NonPairedReads)`);
fgumi had no such reason and folded fragments into the strand groups, where they
were ignored by strand grouping but mis-accounted. Add a NonPairedReads rejection
reason (metrics + ConsensusMetrics field), map the caller-side FragmentRead reason
to it (it previously mis-mapped to SameStrandOnly), and partition + reject
fragments in DuplexConsensusCaller::consensus_reads.
R2-MET-05 (S4): fgbio seeds and always emits per-caller rejection rows via
`initializeRejectCounts` (`UmiConsensusCaller.scala:223-224`) — the duplex caller
emits `raw_reads_rejected_for_{non_paired_reads,single_strand_only,potential_umi_collision}`
even at zero, in addition to the four vanilla rows. fgumi emitted only the four
vanilla rows and put the duplex reasons in a non-zero-only "optional" set. Replace
the fixed core/optional split in ConsensusMetrics::to_kv_metrics with per-caller
seeding driven by a new ConsensusCallerKind {Vanilla, Duplex, Codec}: seeded rows
are always emitted (in fgbio order), and fgumi's finer-grained reasons are emitted
only when non-zero. Re-key the collision reason from `duplicate_umi` to fgbio's
`potential_umi_collision` and align the three newly seeded rows' descriptions with
fgbio.
(This is the row-set half of R2-MET-05; #504 owns the remaining description-text
parity. Codec does not currently emit KV stats, so codec seeding is defined but
unexercised; simplex already matched fgbio's four vanilla rows.)
Verified fgbio<->fgumi end to end
(reports/fgbio-parity-fixtures/r2-met-05-ucc-01-duplex-rejection-rows.sh):
- R2-MET-05: fgbio duplex emits 7 rejection rows; fgumi BEFORE emitted 4; fgumi
AFTER emits the same 7 rows (keys match fgbio exactly).
- R2-UCC-01: with one fragment read, fgbio reports non_paired_reads=1; fgumi
BEFORE emitted no such row; fgumi AFTER reports non_paired_reads=1.
…3-04, DXM3-06, SIMM3-02) Completes the metrics-parity work #504 started. - DXM3-04: #504 wired the fgbio-style float formatter onto the fgumi-metrics crate's metric structs but left `DedupMetricsOutput.duplicate_rate` (in fgumi_lib) on raw serde f64. Make `fgumi_metrics::float` public and wire it there so dedup's one fraction column formats like the rest (a whole-number rate writes `1`, not `1.0`). (Verified in re-scoping that the audit's "clip" struct has no f64 fields, and group's `avg_reads_per_molecule` is `#[serde(skip)]`, so dedup is the only serialized gap.) - DXM3-06: `fgumi duplex-metrics` accepted `--min-ab-reads 0`, silently labeling every tag family a duplex. fgbio's CollectDuplexSeqMetrics validates `minAbReads >= 1`; add the same guard (message matches fgbio's wording). - SIMM3-02: `fgumi simplex-metrics` accepted `--min-reads 0` (every family of size >= 0 labeled a consensus family). Add the `min_reads >= 1` guard. Real-tool parity: fgbio source (CollectDuplexSeqMetrics.scala:288) confirms `validate(minAbReads >= 1, ...)`; fgumi now errors identically on 0. Tests: dedup float-format serialization + duplex/simplex zero-threshold reject. Stacked on #504. Tracker: reports/2026-07-09-fgumi-final-audit-burndown-tracker.md (W3).
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-metrics/src/consensus.rs`:
- Around line 60-65: Add a regression test for the metric type containing
avg_input_reads_per_consensus and avg_raw_read_depth, exercising NaN and
Infinity round-trips through write_metrics and read_metrics. Assert both fields
preserve their values, consistent with
correct.rs::test_nan_infinity_serialization. Also add the required
programmatically generated identity-or-documented-divergence test against the
fgbio baseline for this metric-format change.
In `@crates/fgumi-metrics/src/writer.rs`:
- Around line 79-90: Update write_header_only to set the temporary file’s
permissions to match those used by the direct populated-file write path before
calling tmp.persist(path_ref). Preserve the existing atomic temp-file workflow
and apply the mode normalization after rewriting the header but before renaming.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48037dc2-8c1f-42ff-b824-1d7d0069b7ee
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (17)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/correct.rscrates/fgumi-metrics/src/duplex.rscrates/fgumi-metrics/src/float.rscrates/fgumi-metrics/src/group.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/rejection.rscrates/fgumi-metrics/src/shared.rscrates/fgumi-metrics/src/simplex.rscrates/fgumi-metrics/src/writer.rssrc/lib/commands/clip.rssrc/lib/commands/duplex_metrics.rssrc/lib/commands/group.rssrc/lib/commands/simplex_metrics.rssrc/lib/metrics/mod.rstests/integration/test_group_command.rs
ebc51e1 to
fda0a20
Compare
…ows (CONS-01, DUP-01, R2-MET-05, R2-UCC-01) (#508) * fix(consensus): guard sort order and duplex error rates (CONS-01, DUP-01) Two missing input guards on the standalone consensus callers, both matching fgbio's UmiConsensusCaller / CallDuplexConsensusReads behavior. CONS-01 (S3): fgbio's UmiConsensusCaller.checkSortOrder (UmiConsensusCaller.scala:165-173) errors unless the input is template-coordinate sorted; simplex/duplex/codec had no sort-order guard (only group/dedup did), so coordinate-sorted standalone input silently interleaved molecules and every molecule was split — corrupting the entire output. Add check_consensus_sort_order (commands/common.rs) mirroring checkSortOrder's three cases exactly (UmiConsensusCallerTest.scala:34-60): template-coordinate => accept; SO:unsorted + GO:query (no SS) => warn; anything else => error. It reads only the already-read header and never re-opens the stream, so it is safe on stdin. Wired into both the threaded and single-threaded header-read sites of simplex/duplex/codec. New fgumi-sam helpers is_query_grouped_unsorted and header_as_template_coordinate back the guard and the test-input builder. DUP-01 (S3): fgbio's CallDuplexConsensusReads validates errorRate{Pre,Post}Umi > 0 (CallDuplexConsensusReads.scala:131-132) — Q0 means P(err)=1, a nonsensical prior. fgumi duplex accepted a raw u8 with no check. Add Duplex::validate() rejecting a zero pre/post-UMI error rate. (codec already guarded this; simplex correctly allows >=0 per fgbio, and its u8 is never negative.) Consensus command tests now stamp template-coordinate order on their input BAMs (a new SamBuilder::set_template_coordinate_sort_order helper, plus the codec / duplex input-writer helpers), matching fgbio's command tests which build input with SamOrder.TemplateCoordinate. Verified fgbio<->fgumi end to end (reports/fgbio-parity-fixtures/ cons-01-dup-01-consensus-guards.sh): - CONS-01, coordinate input: fgbio exit 1 (reject); fgumi BEFORE exit 0 (silently accepted); fgumi AFTER exit 1 (rejects, same message class); template-coordinate input AFTER exit 0. - DUP-01, --error-rate-pre/post-umi 0: fgbio exit 1; fgumi BEFORE exit 0; fgumi AFTER exit 1; defaults AFTER exit 0. * fix(consensus): seed duplex KV rejection rows and reject fragments (R2-MET-05, R2-UCC-01) Bring the duplex caller's KV consensus statistics in line with fgbio's per-caller rejection-row model. R2-UCC-01 (S4): fgbio's DuplexConsensusCaller partitions unpaired/fragment reads out and rejects them as NonPairedReads (`val (pairs, frags) = recs.partition(_.paired); rejectRecords(frags, NonPairedReads)`); fgumi had no such reason and folded fragments into the strand groups, where they were ignored by strand grouping but mis-accounted. Add a NonPairedReads rejection reason (metrics + ConsensusMetrics field), map the caller-side FragmentRead reason to it (it previously mis-mapped to SameStrandOnly), and partition + reject fragments in DuplexConsensusCaller::consensus_reads. R2-MET-05 (S4): fgbio seeds and always emits per-caller rejection rows via `initializeRejectCounts` (`UmiConsensusCaller.scala:223-224`) — the duplex caller emits `raw_reads_rejected_for_{non_paired_reads,single_strand_only,potential_umi_collision}` even at zero, in addition to the four vanilla rows. fgumi emitted only the four vanilla rows and put the duplex reasons in a non-zero-only "optional" set. Replace the fixed core/optional split in ConsensusMetrics::to_kv_metrics with per-caller seeding driven by a new ConsensusCallerKind {Vanilla, Duplex, Codec}: seeded rows are always emitted (in fgbio order), and fgumi's finer-grained reasons are emitted only when non-zero. Re-key the collision reason from `duplicate_umi` to fgbio's `potential_umi_collision` and align the three newly seeded rows' descriptions with fgbio. (This is the row-set half of R2-MET-05; #504 owns the remaining description-text parity. Codec does not currently emit KV stats, so codec seeding is defined but unexercised; simplex already matched fgbio's four vanilla rows.) Verified fgbio<->fgumi end to end (reports/fgbio-parity-fixtures/r2-met-05-ucc-01-duplex-rejection-rows.sh): - R2-MET-05: fgbio duplex emits 7 rejection rows; fgumi BEFORE emitted 4; fgumi AFTER emits the same 7 rows (keys match fgbio exactly). - R2-UCC-01: with one fragment read, fgbio reports non_paired_reads=1; fgumi BEFORE emitted no such row; fgumi AFTER reports non_paired_reads=1.
fda0a20 to
67423d1
Compare
…3-04, DXM3-06, SIMM3-02) Completes the metrics-parity work #504 started. - DXM3-04: #504 wired the fgbio-style float formatter onto the fgumi-metrics crate's metric structs but left `DedupMetricsOutput.duplicate_rate` (in fgumi_lib) on raw serde f64. Make `fgumi_metrics::float` public and wire it there so dedup's one fraction column formats like the rest (a whole-number rate writes `1`, not `1.0`). (Verified in re-scoping that the audit's "clip" struct has no f64 fields, and group's `avg_reads_per_molecule` is `#[serde(skip)]`, so dedup is the only serialized gap.) - DXM3-06: `fgumi duplex-metrics` accepted `--min-ab-reads 0`, silently labeling every tag family a duplex. fgbio's CollectDuplexSeqMetrics validates `minAbReads >= 1`; add the same guard (message matches fgbio's wording). - SIMM3-02: `fgumi simplex-metrics` accepted `--min-reads 0` (every family of size >= 0 labeled a consensus family). Add the `min_reads >= 1` guard. Real-tool parity: fgbio source (CollectDuplexSeqMetrics.scala:288) confirms `validate(minAbReads >= 1, ...)`; fgumi now errors identically on 0. Tests: dedup float-format serialization + duplex/simplex zero-threshold reject. Stacked on #504. Tracker: reports/2026-07-09-fgumi-final-audit-burndown-tracker.md (W3).
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-metrics/src/writer.rs`:
- Around line 53-96: Update write_metrics so populated metrics also serialize
through a temporary file in the destination directory and atomically persist it
to path_ref, matching write_header_only’s temp-file workflow. Preserve the
existing empty-metrics header behavior and contextual error messages, and ensure
the temporary file uses the same permissions approach as the current direct
write.
In `@src/lib/commands/clip.rs`:
- Around line 305-333: The per-template clipping and mate-repair workflow is
duplicated between execute_single_threaded and the process_fn path. Extract the
shared primary-pair and lone-fragment logic—including clip thresholds/flags,
overlap and mate-extension metrics, set_mate_info_raw, and
fix_supplemental_mate_info—into a free function accepting plain configuration
values rather than &self, then call it from both paths while preserving current
record selection and outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 499a2567-2367-4db7-b3d6-162c05e145f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (17)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/correct.rscrates/fgumi-metrics/src/duplex.rscrates/fgumi-metrics/src/float.rscrates/fgumi-metrics/src/group.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/rejection.rscrates/fgumi-metrics/src/shared.rscrates/fgumi-metrics/src/simplex.rscrates/fgumi-metrics/src/writer.rssrc/lib/commands/clip.rssrc/lib/commands/duplex_metrics.rssrc/lib/commands/group.rssrc/lib/commands/simplex_metrics.rssrc/lib/metrics/mod.rstests/integration/test_group_command.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-metrics/src/writer.rs`:
- Around line 53-96: Update write_metrics so populated metrics also serialize
through a temporary file in the destination directory and atomically persist it
to path_ref, matching write_header_only’s temp-file workflow. Preserve the
existing empty-metrics header behavior and contextual error messages, and ensure
the temporary file uses the same permissions approach as the current direct
write.
In `@src/lib/commands/clip.rs`:
- Around line 305-333: The per-template clipping and mate-repair workflow is
duplicated between execute_single_threaded and the process_fn path. Extract the
shared primary-pair and lone-fragment logic—including clip thresholds/flags,
overlap and mate-extension metrics, set_mate_info_raw, and
fix_supplemental_mate_info—into a free function accepting plain configuration
values rather than &self, then call it from both paths while preserving current
record selection and outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 499a2567-2367-4db7-b3d6-162c05e145f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (17)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/correct.rscrates/fgumi-metrics/src/duplex.rscrates/fgumi-metrics/src/float.rscrates/fgumi-metrics/src/group.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/rejection.rscrates/fgumi-metrics/src/shared.rscrates/fgumi-metrics/src/simplex.rscrates/fgumi-metrics/src/writer.rssrc/lib/commands/clip.rssrc/lib/commands/duplex_metrics.rssrc/lib/commands/group.rssrc/lib/commands/simplex_metrics.rssrc/lib/metrics/mod.rstests/integration/test_group_command.rs
🛑 Comments failed to post (2)
crates/fgumi-metrics/src/writer.rs (1)
53-96: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Only the empty-metrics path got atomic-write hardening; the populated path (the common case) is still a direct in-place write.
A crash mid-
write_tsvon a non-empty metrics file leaves a truncated/corrupt TSV on disk — the same class of problemwrite_header_only's temp-file+persistwas built to avoid, just for the branch that matters more in practice.♻️ Sketch: reuse the temp-file+persist pattern for both branches
pub fn write_metrics<P: AsRef<Path>, T: Serialize + Default>( path: P, metrics: &[T], description: &str, ) -> Result<()> { let path_ref = path.as_ref(); - if metrics.is_empty() { - return write_header_only::<_, T>(path_ref).with_context(|| { - format!("Failed to write {} metrics header: {}", description, path_ref.display()) - }); - } - DelimFile::default() - .write_tsv(path_ref, metrics) - .with_context(|| format!("Failed to write {} metrics: {}", description, path_ref.display())) + let dir = path_ref.parent().unwrap_or_else(|| Path::new(".")); + let tmp = tempfile::Builder::new().make_in(dir, std::fs::File::create)?; + if metrics.is_empty() { + write_header_only_into(tmp.path())?; + } else { + DelimFile::default().write_tsv(tmp.path(), metrics)?; + } + tmp.persist(path_ref) + .with_context(|| format!("Failed to write {} metrics: {}", description, path_ref.display())) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fgumi-metrics/src/writer.rs` around lines 53 - 96, Update write_metrics so populated metrics also serialize through a temporary file in the destination directory and atomically persist it to path_ref, matching write_header_only’s temp-file workflow. Preserve the existing empty-metrics header behavior and contextual error messages, and ensure the temporary file uses the same permissions approach as the current direct write.src/lib/commands/clip.rs (1)
305-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Clip+mate-repair logic duplicated between single- and multi-threaded paths.
The primary-pair clipping +
set_mate_info_raw/fix_supplemental_mate_infosequence is implemented twice (here and inClip::clip_pair/clip_fragment), and this very diff had to add the identical mate-info-fixing step to both copies. Any future correctness fix to pair clipping or mate-info repair now needs to be applied in two places, risking silent divergence between single- and multi-threaded output.Consider extracting the shared per-template body (threshold clipping + overlap/extend clipping +
set_mate_info_raw+fix_supplemental_mate_info) into a free function taking plain values (clipper, thresholds, flags) instead of&self, callable from bothexecute_single_threadedand theprocess_fnclosure.Also applies to: 635-709
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/commands/clip.rs` around lines 305 - 333, The per-template clipping and mate-repair workflow is duplicated between execute_single_threaded and the process_fn path. Extract the shared primary-pair and lone-fragment logic—including clip thresholds/flags, overlap and mate-extension metrics, set_mate_info_raw, and fix_supplemental_mate_info—into a free function accepting plain configuration values rather than &self, then call it from both paths while preserving current record selection and outcomes.
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.
67423d1 to
a3e7ff5
Compare
|
Addressed the two actionable findings from the latest CodeRabbit review (they came through only in the review-body "Prompt for AI agents" block, since the inline comments failed to post):
|
…3-04, DXM3-06, SIMM3-02) Completes the metrics-parity work #504 started. - DXM3-04: #504 wired the fgbio-style float formatter onto the fgumi-metrics crate's metric structs but left `DedupMetricsOutput.duplicate_rate` (in fgumi_lib) on raw serde f64. Make `fgumi_metrics::float` public and wire it there so dedup's one fraction column formats like the rest (a whole-number rate writes `1`, not `1.0`). (Verified in re-scoping that the audit's "clip" struct has no f64 fields, and group's `avg_reads_per_molecule` is `#[serde(skip)]`, so dedup is the only serialized gap.) - DXM3-06: `fgumi duplex-metrics` accepted `--min-ab-reads 0`, silently labeling every tag family a duplex. fgbio's CollectDuplexSeqMetrics validates `minAbReads >= 1`; add the same guard (message matches fgbio's wording). - SIMM3-02: `fgumi simplex-metrics` accepted `--min-reads 0` (every family of size >= 0 labeled a consensus family). Add the `min_reads >= 1` guard. Real-tool parity: fgbio source (CollectDuplexSeqMetrics.scala:288) confirms `validate(minAbReads >= 1, ...)`; fgumi now errors identically on 0. Tests: dedup float-format serialization + duplex/simplex zero-threshold reject. Stacked on #504. Tracker: reports/2026-07-09-fgumi-final-audit-burndown-tracker.md (W3).
…3-04, DXM3-06, SIMM3-02) (#520) Completes the metrics-parity work #504 started. - DXM3-04: #504 wired the fgbio-style float formatter onto the fgumi-metrics crate's metric structs but left `DedupMetricsOutput.duplicate_rate` (in fgumi_lib) on raw serde f64. Make `fgumi_metrics::float` public and wire it there so dedup's one fraction column formats like the rest (a whole-number rate writes `1`, not `1.0`). (Verified in re-scoping that the audit's "clip" struct has no f64 fields, and group's `avg_reads_per_molecule` is `#[serde(skip)]`, so dedup is the only serialized gap.) - DXM3-06: `fgumi duplex-metrics` accepted `--min-ab-reads 0`, silently labeling every tag family a duplex. fgbio's CollectDuplexSeqMetrics validates `minAbReads >= 1`; add the same guard (message matches fgbio's wording). - SIMM3-02: `fgumi simplex-metrics` accepted `--min-reads 0` (every family of size >= 0 labeled a consensus family). Add the `min_reads >= 1` guard. Real-tool parity: fgbio source (CollectDuplexSeqMetrics.scala:288) confirms `validate(minAbReads >= 1, ...)`; fgumi now errors identically on 0. Tests: dedup float-format serialization + duplex/simplex zero-threshold reject. Stacked on #504. Tracker: reports/2026-07-09-fgumi-final-audit-burndown-tracker.md (W3).
Closes #498 (metrics-format items) — makes fgumi's metrics
.txtoutputs readable by fgbio'sMetric.readand matches fgbio's rows, column names/order, and value tokens on a shared input.What changed
floatserde module writes f64 fields as fgbio'sInfinity/-Infinity/NaNtokens (integral values without a trailing.0); applied to every serialized f64 field acrosscorrect/shared/consensus/simplex/duplex/group. Previously onlycorrect.rswas inf-safe; the rest emitted ryuinf/-inf, which fgbio'sDouble.parseDoublecannot read.write_metricsnow always emits the header, even for a zero-row slice; the duplex-metrics / simplex-metrics per-family/UMI/yield writes route through it.UmiGroupingMetricswrong columnsUmiGroupingMetric(accepted_sam_records,discarded_non_pf,discarded_poor_alignment,discarded_ns_in_umi,discarded_umis_to_short). The fgumi-internal summary fields stay in memory for logging but are#[serde(skip)]-ed (derivable; the family distribution already lives in.family_sizes.txt).family_sizesdense→sparsefamilySizeMetrics) instead of a dense1..=maxrange.duplex_umi_countsnondeterministicumias a deterministic secondary sort key so ties no longer fall back to HashMap iteration order.format_floatdoc-comment that falsely claimed byte-format parity with fgbio. Numeric equality is the contract.CorrectUmiszero-count seeding (issue item 5) already reproduces as fixed onmain— both correct execution paths pre-seed a row per expected UMI plus the all-N sentinel — so no change was needed there.fgbio-oracle evidence
reports/fgbio-parity-fixtures/met-02-03-04-fgbio-read.shfeeds fgumi's exact output bytes (proven by the unit tests) to fgbio 4.1.0's realMetric.read:The negative control confirms the old ryu
inftoken was genuinely unreadable by fgbio.Deferred (out of scope here)
MET-05's caller-type-aware row-set seeding — always emitting
raw_reads_rejected_for_non_paired_reads,..._potential_umi_collision, and the codec-specific reasons per fgbio'susedByVanilla/usedByDuplex/usedByCodecmodel — is deferred to the consensus PR. It requires addingRejectionReasonvariants and wiring them into the duplex/codec callers (this is the same behavioral change as R2-UCC-01, "fgbio rejects fragments asNonPairedReads"), which belongs with the consensus work rather than a metrics-format PR. Only the caller-independent description-text parity is done here.Testing
UmiGroupingMetricheader, sparse family sizes across a gap, deterministic duplex-UMI tie order, rejection-text parity.cargo ci-fmt/cargo ci-lintclean; full suite 2215 passed.Summary by CodeRabbit
New Features
NaN,Infinity, and-Infinity.Bug Fixes
Tests