fix(simulate): emit faithful duplex consensus tags - #603
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 (1)
WalkthroughDuplex consensus generation now enforces ChangesDuplex consensus tags
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #603 +/- ##
==========================================
- Coverage 93.42% 93.42% -0.01%
==========================================
Files 175 175
Lines 104807 104935 +128
==========================================
+ Hits 97919 98031 +112
- Misses 6888 6904 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@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 `@src/lib/commands/simulate/consensus_reads.rs`:
- Around line 2002-2046: The invariant test
test_duplex_strand_minimums_sum_to_combined_minimum should use proptest instead
of iterating over 64 fixed seeds. Convert the seed-driven assertions into a
proptest property with generated seeds or relevant inputs, preserving all
existing truth/emitted and strand-sum invariants while allowing failing cases to
shrink.
🪄 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: a863afb3-d89e-4c53-9115-08c7797f62b3
📒 Files selected for processing (1)
src/lib/commands/simulate/consensus_reads.rs
Two defects made `fgumi simulate consensus-reads --duplex` produce records that no real consensus caller would emit, so simulated duplex BAMs were unsound fixtures for validating `fgumi filter` and for fgbio-parity harnesses. Combined per-base arrays on duplex records. `CD_BASES`/`CE_BASES` were written unconditionally, but `duplex_caller` emits only the per-strand `AD_BASES`/ `AE_BASES`/`BD_BASES`/`BE_BASES` arrays alongside the scalar cD/cM/cE. Emit the combined arrays only in simplex mode. This is safe for the documented "suitable for input to `fgumi filter`" contract: filter branches on `is_duplex_consensus` and masks duplex reads through `mask_duplex_bases`, which reads the four per-strand arrays and never the combined ones, while simplex reads still get the `CD_BASES`/`CE_BASES` that `mask_bases` requires. Derive the duplex cD/cM/cE scalars from the per-strand sums rather than from the independently sampled simplex values, matching `duplex_caller`, which computes them over the combined per-base depth (ab_i + ba_i). Strand minimum depths. `aM` and `bM` were each computed as `strand_depth.min(cM)`, so both could equal `cM` and `aM + bM` routinely exceeded it. A duplex position's combined depth is the sum of its two strand depths, so the truth TSV and the BAM tags disagreed with each other and anyone validating a real run against `--truth` got impossible expectations. Split `cM` by the same strand fraction already used for `cD`, clamping the A share to `[cM - bD, min(aD, cM)]` — feasible because `cM <= cD == aD + bD` — which guarantees `aM + bM == cM`, `aM <= aD`, and `bM <= bD`. The combined scalars are summed from the per-strand scalars rather than reduced over the per-base arrays: the two agree only when the arrays carry the min anchor, which `per_base_arrays` adds solely for `read_len >= 2`, so a `--read-length 1` run would have reported `cM == cD` and drifted from the truth TSV again. A proptest over generated seeds (the strand fraction is sampled per read) asserts the summation and ordering invariants, an `rstest` case table pins the truth/tag agreement at read lengths 1, 2, and 50, and further tests assert duplex records carry no `CD_BASES`/`CE_BASES` while retaining all four per-strand arrays, and that simplex records still carry the combined arrays.
785ce43 to
aff6d77
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Two defects made
fgumi simulate consensus-reads --duplexproduce records that no real consensus caller emits, so simulated duplex BAMs were unsound fixtures for validatingfgumi filterand for fgbio-parity harnesses.Combined per-base arrays on duplex records
CD_BASES/CE_BASESwere written unconditionally, butduplex_calleremits only the per-strandAD_BASES/AE_BASES/BD_BASES/BE_BASESarrays alongside the scalarcD/cM/cE(duplex_caller.rs:1132-1177vs:1207-1209). Now emitted only in simplex mode.This is safe for the documented "suitable for input to
fgumi filter" contract: filter branches onis_duplex_consensusand masks duplex reads throughmask_duplex_bases, which reads the four per-strand arrays (crates/fgumi-consensus/src/filter.rs:819-825) and never the combined ones — while simplex reads still get theCD_BASES/CE_BASESthatmask_basesrequires.The duplex
cD/cM/cEscalars are now derived from the per-strand sums rather than the independently sampled simplex values, matchingduplex_caller, which computes them over the combined per-base depth (ab_i + ba_i).Strand minimum depths could exceed the combined minimum
aMandbMwere each computed asstrand_depth.min(cM), so both could equalcMandaM + bMroutinely exceeded it. A duplex position's combined depth is the sum of its two strand depths, so the truth TSV and the BAM tags disagreed with each other — anyone validating a real run against--truthgot impossible expectations.cMis now split by the same strand fraction already used forcD, clamping the A share to[cM - bD, min(aD, cM)]. That range is feasible becausecM <= cD == aD + bD, and it guaranteesaM + bM == cM,aM <= aD, andbM <= bD.Testing
Tests sweep 64 seeds — the strand fraction is sampled per read, so a single seed proves little — asserting the summation and ordering invariants, and that the truth tuple's
cD/cMagree with the emitted tags (the truth file carries sampled values while the tags are now derived; only theaM + bM == cMinvariant keeps them equal, so it is pinned explicitly). A second test asserts duplex records carry noCD_BASES/CE_BASESwhile retaining all four per-strand arrays, and that simplex records still carry the combined arrays.cargo ci-test: 5536 passed, 22 skippedcargo ci-fmt,cargo ci-lint: cleanSummary by CodeRabbit