fix: Triangular::pdf returns NaN when mode equals min by jaideeppyne · Pull Request #460 · statrs-dev/statrs

jaideeppyne · 2026-08-28T02:00:56Z

Triangular::new accepts mode == min (a right triangle) — 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. This handles 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. Added pdf/ln_pdf cases for both endpoint modes and extended test_continuous to cover them; both new assertions fail on main with 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
- Fixed triangular distribution calculations when the mode equals the minimum value.
- Prevented invalid results at the distribution’s rising-edge boundary.
- Improved reliability for probability density and log-density calculations in this edge case.
Tests
- Added coverage for minimum-mode endpoint behavior.
- Verified accurate density and log-density results.
- Expanded validation of continuous distribution behavior at endpoint modes.

`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.

coderabbitai · 2026-08-28T02:01:44Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 876248a4-10bc-4a46-a38b-e56efcdd5c1b

📥 Commits

Reviewing files that changed from the base of the PR and between 70402ba and 211756d.

📒 Files selected for processing (1)

src/distribution/triangular.rs

💤 Files with no reviewable changes (1)

src/distribution/triangular.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

The triangular PDF now handles mode == min without evaluating a degenerate denominator. Tests cover endpoint PDF values, logarithmic PDF values, and continuous-distribution validation for both boundary modes.

Changes

Triangular distribution endpoint handling

Layer / File(s)	Summary
Endpoint density and validation `src/distribution/triangular.rs`	`Triangular::pdf` returns `2 / (max - min)` when `mode == min`. Tests cover PDF values, `ln_pdf`, and continuous-distribution validation for boundary modes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 21175

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)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing `Triangular::pdf` when `mode == min` causes `NaN`.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 1 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

day01 · 2026-08-28T09:34:56Z

        if a <= x && x <= c {
-            2.0 * (x - a) / ((b - a) * (c - a))
+            if c == a {
+                // The only `x` reaching this branch is `x == min == mode`, where the


Delete the comment. The c == a guard plus 2.0 / (b - a) is self-explanatory once the doc formula mentions the special case

Removed. The doc formula above already states the mode == min case, so the guard stands on its own.

day01 · 2026-08-28T09:36:01Z

        test_exact(-5.0, -3.0, -4.0, 0.5, pdf(-4.5));
        test_exact(-5.0, -3.0, -4.0, 1.0, pdf(-4.0));
        test_exact(-5.0, -3.0, -4.0, 0.5, pdf(-3.5));
+        // mode == min: the rising edge is degenerate at x == min, the density there


unnecessary comments, if you prefer present different behaviour it is for different test case there should be explanation, it shouldnt be in comment in source code imo

Fair, moved rather than commented. The degenerate-edge assertions are now in test_pdf_mode_at_an_endpoint and test_ln_pdf_mode_at_an_endpoint, so the case is named instead of explained, and test_pdf goes back to what it was. I also added the mode == max mirror to both while moving them.

Both new tests fail on master and pass with the change.

day01

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.

jaideeppyne · 2026-08-28T10:35:34Z

Point taken on the comment volume, that was overdone. Both are gone now.

The in-body comment in pdf is deleted, and the endpoint assertions moved into test_pdf_mode_at_an_endpoint and test_ln_pdf_mode_at_an_endpoint rather than sitting behind a comment in test_pdf. While moving them I added the mode == max mirror to both. The only prose left is the one line in the doc formula, which I kept since you referred to it as covering the special case, happy to cut that too.

The code change itself is unchanged, and both new tests fail on master.

day01

Results correct, algo confirms with mapmath

day01 · 2026-08-28T11:07:12Z

@jaideeppyne Yeah, cut the narrative! XD

jaideeppyne · 2026-08-28T14:05:30Z

Ha, fair. Cut that one too, the doc block is back to just the formula.

day01 reviewed Aug 28, 2026

View reviewed changes

day01 suggested changes Aug 28, 2026

View reviewed changes

Drop the explanatory comments and split out the endpoint tests

70402ba

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.

day01 approved these changes Aug 28, 2026

View reviewed changes

Cut the doc note too

211756d

Navigation Menu

Uh oh!

fix: Triangular::pdf returns NaN when mode equals min - #460

fix: Triangular::pdf returns NaN when mode equals min#460
jaideeppyne wants to merge 3 commits into
statrs-dev:mainfrom
jaideeppyne:fix/triangular-pdf-mode-equals-min

jaideeppyne commented Aug 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

day01 Aug 28, 2026

Uh oh!

jaideeppyne Aug 28, 2026

Uh oh!

day01 Aug 28, 2026

Uh oh!

jaideeppyne Aug 28, 2026

Uh oh!

day01 left a comment

Uh oh!

jaideeppyne commented Aug 28, 2026

Uh oh!

day01 left a comment

Uh oh!

day01 commented Aug 28, 2026

Uh oh!

jaideeppyne commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jaideeppyne commented Aug 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

day01 Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jaideeppyne Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

day01 Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jaideeppyne Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

day01 left a comment

Choose a reason for hiding this comment

Uh oh!

jaideeppyne commented Aug 28, 2026

Uh oh!

day01 left a comment

Choose a reason for hiding this comment

Uh oh!

day01 commented Aug 28, 2026

Uh oh!

jaideeppyne commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jaideeppyne commented Aug 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading