Rearrange left/right DCB tail integral evaluation for numerical stability when n1 or n2 is very close to 1#1253
Conversation
…lity when n1 or n2 is very close to 1
📝 WalkthroughWalkthroughThe PR refactors power computation factorization in the ChangesTail Integral Power Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes involve mathematical computation refactoring across two coupled integral tail regions with high code block complexity. Verification requires careful inspection of exponent handling, term factorization, and mathematical equivalence between old and new power computation patterns. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/RooDoubleCBFast.cc (1)
141-149: ⚡ Quick winAdd a regression for the just-above-threshold tail path.
This rewrite targets a very specific instability, but nothing here locks in the
fabs(n?-1) > 1e-5branch that was blowing up before. Please add a test withn1/n2just above the cutoff and an integration range fully in the corresponding tail, asserting the integral stays finite and matches the logarithmic form closely.Also applies to: 178-186
🤖 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/RooDoubleCBFast.cc` around lines 141 - 149, The tail-path math for left1/left2 (and the analogous right-side code around lines with n2) becomes unstable when n1 or n2 are just above 1; add a small-epsilon branch to RooDoubleCBFast that detects |n1-1|<1e-5 (and similarly |n2-1|<1e-5) and uses the logarithmic/limit form (use exp((1.-n1)*log(term)) or the first-order log expansion) instead of direct fast_pow to compute left1/left2 (symbols: left1, left2, n1, n2, n1invalpha1, alpha1, tmin, thigh and the matching right-side variables), and add a regression unit test that sets n1 (and separately n2) just above 1 with an integration range fully inside the tail and asserts the integral is finite and matches the expected logarithmic-limit value within tolerance.
🤖 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.
Nitpick comments:
In `@src/RooDoubleCBFast.cc`:
- Around line 141-149: The tail-path math for left1/left2 (and the analogous
right-side code around lines with n2) becomes unstable when n1 or n2 are just
above 1; add a small-epsilon branch to RooDoubleCBFast that detects |n1-1|<1e-5
(and similarly |n2-1|<1e-5) and uses the logarithmic/limit form (use
exp((1.-n1)*log(term)) or the first-order log expansion) instead of direct
fast_pow to compute left1/left2 (symbols: left1, left2, n1, n2, n1invalpha1,
alpha1, tmin, thigh and the matching right-side variables), and add a regression
unit test that sets n1 (and separately n2) just above 1 with an integration
range fully inside the tail and asserts the integral is finite and matches the
expected logarithmic-limit value within tolerance.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (98.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1253 +/- ##
=======================================
Coverage 20.90% 20.90%
=======================================
Files 195 195
Lines 26316 26316
Branches 3947 3947
=======================================
Hits 5502 5502
Misses 20814 20814
🚀 New features to boost your workflow:
|
1 participant
When using RooDoubleCBFast, we noticed that the raw, unnormalized integral (which is returned by
RooDoubleCBFast::analyticalIntegral) of the left (right) power-law tail blows up/is unstable whenn1(n2) is very close to 1 -- but not close enough that the integral is given by the logarithm in the code.In the attached slides , I show that the current threshold in the code where the log is used for the integral,
(n1 - 1) < 1e-5(same forn2), does not need to be changed to fix this issue. It turns out that the fix is simply to rearrange the evaluation of line 186 for the right tail and line 149 for the left tail to be done in an algebraically equivalent, but more numerically stable way. This is done in the commit.See the attached slides for: