Repository navigation
fix(consensus): sort-order + error-rate guards and duplex rejection rows (CONS-01, DUP-01, R2-MET-05, R2-UCC-01) - #508
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughConsensus commands now validate template-coordinate input, duplex calling rejects fragment reads, and metrics expose caller-specific rejection rows. SAM builders and tests stamp required headers, while duplex validates UMI error-rate arguments. ChangesConsensus pipeline updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant Header
participant ConsensusPipeline
Command->>Header: read BAM ordering metadata
Command->>ConsensusPipeline: check_consensus_sort_order
ConsensusPipeline-->>Command: accept input or return an error
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
d881678 to
98be98d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #508 +/- ##
==========================================
+ Coverage 92.52% 92.56% +0.04%
==========================================
Files 165 165
Lines 99259 99634 +375
==========================================
+ Hits 91839 92230 +391
+ Misses 7420 7404 -16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
98be98d to
a24cb2a
Compare
a24cb2a to
e80c30b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
e80c30b to
37e2e83
Compare
…-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.
37e2e83 to
d52374b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-consensus/src/duplex_caller.rs`:
- Around line 709-713: The consensus tests cover only fragment-only MI groups;
add a mixed-family test containing paired reads and stray fragments. Using the
existing consensus test helpers and assertions near
test_consensus_reads_rejects_fragments_as_non_paired and
test_not_create_records_from_fragments, verify paired reads still generate a
duplex consensus while fragments are classified and counted as NonPairedReads.
🪄 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: 307fd7b0-5a02-4c90-92fd-e0641e707f90
📒 Files selected for processing (12)
crates/fgumi-consensus/src/caller.rscrates/fgumi-consensus/src/duplex_caller.rscrates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/rejection.rscrates/fgumi-sam/src/builder.rscrates/fgumi-sam/src/lib.rssrc/lib/commands/codec.rssrc/lib/commands/common.rssrc/lib/commands/duplex.rssrc/lib/commands/simplex.rstests/integration/test_duplex_command.rs
d52374b to
0fe810a
Compare
0fe810a to
ae3dee2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 637-666: Update test_to_kv_metrics_duplex_seeds_duplex_rows to
assert the complete ordered key sequence returned by to_kv_metrics for both
Duplex and Vanilla, rather than checking keys only with any(). Include every
expected row in documented fgbio order, while preserving the existing
duplex-only inclusion and Vanilla exclusion checks.
🪄 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: c50c165e-3a05-4c44-8fd9-fa0a15f8164d
📒 Files selected for processing (8)
crates/fgumi-consensus/src/caller.rscrates/fgumi-consensus/src/duplex_caller.rscrates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/rejection.rssrc/lib/commands/duplex.rssrc/lib/commands/simplex.rstests/integration/test_duplex_command.rs
…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.
ae3dee2 to
4332216
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
Burn-down PR #10 of the fgbio↔fgumi behavioral-parity tracker. Four findings, all verified end-to-end against fgbio 4.1.0. Stacked on #505 (
nh/fix-codec-fr-pair-rejection) because the rejection-machinery findings touch the same files (caller.rs,codec_caller.rs,consensus.rs,rejection.rs); retarget tomainonce #505 merges.CONS-01 (S3) — consensus sort-order guard
fgbio's
UmiConsensusCaller.checkSortOrdererrors unless the input is template-coordinate sorted;simplex/duplex/codechad no such guard (onlygroup/dedupdid), so coordinate-sorted standalone input silently interleaved molecules and split every one of them — corrupting the entire output. Addedcheck_consensus_sort_order(mirroring fgbio's three cases exactly: template-coordinate → accept,SO:unsorted+GO:query→ warn, else → error), wired into the already-read header of both the threaded and single-threaded paths of all three callers (stdin-safe; no re-open).DUP-01 (S3) — duplex error-rate validation
fgbio's
CallDuplexConsensusReadsrequireserrorRate{Pre,Post}Umi > 0(Q0 ⇒ P(err)=1).duplexaccepted a rawu8with no check. AddedDuplex::validate()rejecting a zero pre/post-UMI error rate. (codec already guarded this; simplex correctly allows>= 0.)R2-UCC-01 (S4) — reject fragment reads
fgbio's
DuplexConsensusCallerpartitions unpaired/fragment reads out and rejects them asNonPairedReads; fgumi folded them into the strand groups where they were mis-accounted. Added aNonPairedReadsrejection reason and partition/reject them inconsensus_reads.R2-MET-05 (S4) — seed the duplex KV rejection rows
fgbio seeds and always emits per-caller rejection rows (
initializeRejectCounts). The duplex caller emitsraw_reads_rejected_for_{non_paired_reads,single_strand_only,potential_umi_collision}even at zero; fgumi emitted only the four vanilla rows. Replaced the fixed core/optional split with per-caller seeding driven by a newConsensusCallerKind, re-keyed the collision reason to fgbio'spotential_umi_collision, and aligned the three newly seeded rows' descriptions. (Row-set half only; #504 owns the remaining description-text parity.)Parity evidence (fgbio 4.1.0 ↔ fgumi, before → after)
--error-rate-pre/post-umi 0: fgbio exit 1; fgumi BEFORE exit 0; fgumi AFTER exit 1; defaults exit 0.non_paired_reads=1; fgumi BEFORE no such row; fgumi AFTERnon_paired_reads=1.Reproducers:
reports/fgbio-parity-fixtures/cons-01-dup-01-consensus-guards.shandreports/fgbio-parity-fixtures/r2-met-05-ucc-01-duplex-rejection-rows.sh.cargo ci-fmt,cargo ci-lint, andcargo ci-test(2221 tests) all green.Summary by CodeRabbit
New Features
Bug Fixes
Tests