test(consensus): pin that the per-base error count stays exact on deep pileups - #613
Conversation
…p pileups The per-base error count is `contributions() - observations_for_base(base)`, and the callers clamp only the *stored* cD/cE values to fgbio's `Short` ceiling. That is correct, but it is load-bearing and currently implicit: if either operand were narrowed to a saturating `u16`/`i16` counter, the difference would collapse toward zero on a pileup deep enough for both to saturate, silently understating cE on exactly the deepest families. This was raised as a suspected cE-collapse bug. It is not one -- `observations` is `[u32; 4]` and `contributions()` sums it, so neither operand saturates -- but nothing pinned that. This adds a test over a pileup where both the matching and mismatching counts exceed the ceiling and asserts the difference is exact rather than collapsed. No behavior change.
|
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)
WalkthroughAdds a regression test covering deep pileups beyond ChangesDeep pileup regression
Estimated code review effort: 1 (Trivial) | ~5 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 #613 +/- ##
==========================================
- Coverage 93.42% 93.39% -0.04%
==========================================
Files 175 175
Lines 104807 104817 +10
==========================================
- Hits 97919 97896 -23
- Misses 6888 6921 +33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was carried on the port list as a suspected cE collapse on deep mixed pileups — the concern being that the per-base error count subtracts two operands that are already saturated at the
Shortceiling, so on a deep enough pileup the difference would collapse toward zero and understatecE.I investigated and it is not a bug. No behavior change here; this is the test that pins why.
Why it does not happen
The error count is computed as:
Both operands are unsaturated
u32counters —observationsis[u32; 4]andcontributions()sums it. The clamp to fgbio'sShortceiling is applied to the result, when the value is pushed into theerrorsarray, which is exactly what fgbio does (it stores per-base depths and errors asShort). So the subtraction is exact and only the stored value is clamped.Why it is still worth a test
The correctness of
cEon deep families depends entirely on those two operands staying wide, and nothing pinned that. Narrowingobservationsto a saturatingu16— a plausible "memory optimization" — would silently collapse the difference toward zero on exactly the deepest families, where the metric matters most, with no test failing.The test builds a pileup where both the matching and mismatching counts individually exceed the
Shortceiling (70,534 contributions total) and asserts the error count comes back exact at 36,767, rather than the 0 a saturated-operand subtraction would produce.Wave 1 separately verified that per-base
aD/bD/ae/beclamp correctly viaclamp_per_base_to_fgbio_shortwithi64intermediates, so this closes out the last open question in that area.cargo ci-test(5535 tests),cargo ci-fmt,cargo ci-lint,RUSTDOCFLAGS="-D warnings" cargo ci-docpass.Summary by CodeRabbit
Bug Fixes
Tests