Repository navigation
refactor(duplex-metrics): replace dead Binomial::new Err arm with expect - #363
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIn calculate_ideal_duplex_fraction_per_size, Binomial::new(0.5, size as u64) was changed from a match that skipped families on Err to an expect that panics on Err. The code now assumes Binomial::new cannot fail and will abort if it does. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 #363 +/- ##
==========================================
+ Coverage 90.93% 91.00% +0.06%
==========================================
Files 78 78
Lines 51046 51045 -1
==========================================
+ Hits 46421 46451 +30
+ Misses 4625 4594 -31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
`statrs::distribution::Binomial::new(p, n)` returns `BinomialError::ProbabilityInvalid` *only* when `p` is NaN or outside `[0, 1]` (see statrs 0.18 `binomial.rs:67-70`). In `calculate_ideal_duplex_fraction` every call site passes the literal `0.5`, so the `Err` arm is statically unreachable — the existing `Err(_) => continue` was harmless but silently masked the contract that `p` is fixed at 0.5. Replace the `match` with an `.expect(...)` so any future refactor that lets `p` become dynamic surfaces the panic immediately rather than silently dropping families from the ideal-fraction calculation. No behaviour change on any current code path.
cffd59b to
6c98524
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
In
calculate_ideal_duplex_fraction, thematch Binomial::new(0.5, size as u64) { Err(_) => continue, … }arm is statically unreachable —statrs::distribution::Binomial::new(p, n)only returnsErr(BinomialError::ProbabilityInvalid)whenpis NaN or outside[0, 1](seestatrs-0.18.0/src/distribution/binomial.rs:67-70), and every call site here passes the literal0.5.Replace the
matchwith.expect(\"…\")so that any future refactor lettingpbecome dynamic surfaces the panic immediately instead of silently dropping families from the ideal-fraction calculation.Diff
One file, 4 lines net:
Behaviour
None — the replaced arm could not have fired on any input. Spotted during review of #359; sized as a separate PR per reviewer guidance to keep that PR's diff focused on the OOM fix.
Test plan