Skip to content

fix: one more condition in inverse_cdf to account for small enough probabilities.#332

Merged
YeungOnion merged 3 commits intostatrs-dev:masterfrom
geosarr:fix_inverse_cdf
Apr 6, 2025
Merged

fix: one more condition in inverse_cdf to account for small enough probabilities.#332
YeungOnion merged 3 commits intostatrs-dev:masterfrom
geosarr:fix_inverse_cdf

Conversation

@geosarr
Copy link
Contributor

@geosarr geosarr commented Mar 12, 2025

Fix: #330

@PlasmaPower
Copy link

It'd probably be good to add a regression test to this as well

@YeungOnion
Copy link
Contributor

Seems like your condition is the general condition for evaluating to the minimum - it could replace the first condition of checking zero input. And the test would be great.

You need not do the test in the general sense where the default impl is tested on a type that's solely defined for the test, just adding it to those that use the default should suffice.

Let me know if you need anything!

@geosarr
Copy link
Contributor Author

geosarr commented Apr 4, 2025

Thanks for your comments @PlasmaPower and @YeungOnion.

I added some tests. But I noticed that the following test fails.

#[cfg(test)]
mod test {
    use statrs::distribution::{Bernoulli, DiscreteCDF};

    #[test]
    fn test_inverse_cdf() {
        assert_eq!(0, Bernoulli::new(0.5).unwrap().inverse_cdf(0.5));
    }
}

It seems to be due to the implementation of Bernoulli::cdf, which uses the output of function::beta::beta_reg. Currently Bernouilli::cdf(0) is 0.499... instead of 0.5, on my machine.

It may be probably better to use the "exact implementation" as mentionned in the doc of Bernoulli::cdf:

Bernoulli::cdf(x) = if x >= 1 { 1 } else { 1 - p }

@YeungOnion
Copy link
Contributor

I've been wondering when using the binomial distribution to represent the Bernoulli distribution would pose an issue.

Lmk if you'll write the fix and/or refactor for implementing Bernoulli as its own case. If not, I can start and finish that real quick at some time within the next 24 hours.

@geosarr
Copy link
Contributor Author

geosarr commented Apr 5, 2025

@YeungOnion I added the implementation of Bernoulli::cdf.

As a general rule, It think it'd probably be better to use the exact implementations wherever possible, in particular for the statistics like mean, variance, median, skewness, etc of any simple enough distribution.

@codecov
Copy link

codecov bot commented Apr 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.29%. Comparing base (7ef8595) to head (ce9c1f6).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #332      +/-   ##
==========================================
+ Coverage   95.22%   95.29%   +0.06%     
==========================================
  Files          60       60              
  Lines       14254    14205      -49     
==========================================
- Hits        13573    13536      -37     
+ Misses        681      669      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@YeungOnion
Copy link
Contributor

Thanks for making the change.

The only test failure is because #325 is not merged yet. If you're willing, I'd appreciate some feedback on that PR. If not, no pressure.

Thanks for the contribution!

@YeungOnion YeungOnion merged commit 3f7dc5a into statrs-dev:master Apr 6, 2025
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discrete inverse CDF panics if the correct result is the minimum of the distribution

3 participants