-
Notifications
You must be signed in to change notification settings - Fork 103
feat: add specialised inverse cdf implementation for geometric distribution #343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@YeungOnion would you mind taking a look at this? |
|
Thanks for the ping and for contributing! Would you be able to write a few tests, specifically for Since the domain of CDF is natural numbers but the type is u64, want to ensure the inversion property is upheld for the range of values in u64. Does my request seem justified? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #343 +/- ##
==========================================
- Coverage 94.99% 94.98% -0.02%
==========================================
Files 61 61
Lines 13615 13625 +10
==========================================
+ Hits 12934 12942 +8
- Misses 681 683 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Let me know if you need any clarity on the request. |
|
Request is fine, just need to find time to implement! Will try this weekend. |
edcbcab to
1a98f14
Compare
1a98f14 to
af1ad0f
Compare
|
Fixed a rustfmt error and added tests for branch coverage. Struggling with the small p tests because I'm not sure how to generate cases that generate pathological outputs |
|
Yeah, it's kind of tricky to ensure they're extreme. We can take a similar approach for upper bound of p as #155 (comment) For IEEE, largest nontrivial success probability, p, is That should be -- The other extremes of p you might want to check would be if the codomain of the float inverse cdf includes min(intmax, smallest int where forward cdf evaluates to 1), I'll call not reaching this value "early saturation". I'd expect a p near 0 and another near 1. If the test fails somewhere on the interval, I'd include a warning and for the test, the most extreme value for regression testing. To find the most extreme you would likely have to search for the least extreme p where the expression saturates early. If it doesn't fail, then I think you choose the most extreme value that's not degenerate for the forward cdf and check that it saturates correctly. EDIT: math formatting, add a reference for max float less than 1.0 |
|
Hey, did you need anything here? I totally fell off the radar, if you ran into issues, I can work on a test case. I'll probably test include it as a separate pr to see if it even passed with the search implementation. It can be tricky to know if the test is even right for things like this. |
d48bdfb to
81c4a48
Compare
closes #342