-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
Binv remodel #1486
Binv remodel #1486
Conversation
Align BINV algorithm with NUMPY to relax i32::MAX condition to enter BINV algorithm.
This appears to be an alternative to #1484. |
Added n = 1 case. Added np < 10e-10 case. Optimized all BINV branches. Significant performance gains across most of the parameter space. No performance losses for any benchmarks.
Okay, I had the time to sit down tonight and solve the issues both here and with #1484 I believe.
Benchmarks showing significant performance gains across a wide range of benchmarks and no change to two benchmarks (binomial_small as it continues to use the powi() algorithm, and binomial_1e12 because I did not change the BTPE algorithm which this test benchmarks): Original ( binomial/binomial_small binomial/binomial_1 binomial/binomial_10 binomial/binomial_100 binomial/binomial_1000 binomial/binomial_1e12 This (#1486) commit: binomial/binomial binomial/binomial_small binomial/binomial_1 binomial/binomial_10 binomial/binomial_100 binomial/binomial_1000 binomial/binomial_1e12 |
rustfmt loose ends
The
I also think we can unify the code paths. |
CHANGELOG.md
entrySummary
Slightly refactor algorithm to match the Binary Inverse algorithm used by numpy in order to remove the n <= i32::MAX issues.
Motivation
In my use case (large N, small p, seen frequently in computational biology with large N populations of cells with some small mutation rate p, I encountered frequent crashes.
Details
I found the numpy C-implementation of the BINV algorithm and noted they had no i32::MAX constraint to enter. The issue with the current implementation comes as a result of powi() taking i32. I modified the current algorithm to fit the implementation that numpy uses which relaxes this constraint. I tested the new algorithm against a handful of (n,p) values for large n,p and it seems to match what I get in python. I don't think this introduces any new panic/crash conditions, but I may have missed something.