fix: Triangular::pdf returns NaN when mode equals min - #460
Conversation
`Triangular::new` accepts `mode == min` (a right triangle), and
`test_create` already covers it with `create_ok(1.0, 2.0, 1.0)`.
But `pdf` evaluates the rising edge as
2 * (x - min) / ((max - min) * (mode - min))
and at `x == min == mode` both the numerator and the `(mode - min)`
factor are zero, so the result is 0 / 0 = NaN. `ln_pdf` delegates to
`pdf` and returns NaN as well.
let d = Triangular::new(0.0, 1.0, 0.0).unwrap();
d.pdf(0.0); // NaN, expected 2.0
d.ln_pdf(0.0); // NaN, expected ln(2)
The mirrored `mode == max` case is already correct: `pdf` takes the same
first branch and returns `2 / (max - min)`, the height of the triangle.
Handle the degenerate rising edge so both endpoints agree.
`x == min == mode` is the only `x` that can reach that branch when
`mode == min`, so no other input changes value.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe triangular PDF now handles ChangesTriangular distribution endpoint handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized fix corrects endpoint PDF and log-PDF behavior for the triangular distribution and adds regression coverage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
day01
left a comment
day01
left a comment
There was a problem hiding this comment.
too many comments which covers readablity of code
The doc formula already states the mode == min case, and the degenerate edge assertions now live in their own tests instead of behind a comment.
|
Point taken on the comment volume, that was overdone. Both are gone now. The in-body comment in The code change itself is unchanged, and both new tests fail on master. |
day01
left a comment
day01
left a comment
There was a problem hiding this comment.
Results correct, algo confirms with mapmath
|
@jaideeppyne Yeah, cut the narrative! XD |
|
Ha, fair. Cut that one too, the doc block is back to just the formula. |
|
Thanks for the fix! |
3 participants
Triangular::newacceptsmode == min(a right triangle) —test_createalready covers it withcreate_ok(1.0, 2.0, 1.0). Butpdfevaluates the rising edge as2 * (x - min) / ((max - min) * (mode - min)), and atx == min == modeboth the numerator and the(mode - min)factor are zero, so the result is0 / 0= NaN.ln_pdfdelegates topdfand returns NaN as well.The mirrored
mode == maxcase is already correct:pdftakes the same first branch and returns2 / (max - min), the height of the triangle. This handles the degenerate rising edge so both endpoints agree.x == min == modeis the onlyxthat can reach that branch whenmode == min, so no other input changes value. Addedpdf/ln_pdfcases for both endpoint modes and extendedtest_continuousto cover them; both new assertions fail onmainwith NaN.Kept deliberately minimal since you mentioned in #352 that review bandwidth is the bottleneck.
Disclosure: this was AI assisted. An agent ran a differential sweep of the distributions against mpmath at 60 digits, which is what surfaced this, and drafted the patch; I verified the result and the reasoning before submitting.
Summary by CodeRabbit
Bug Fixes
Tests