Closed andreasnoack closed 2 years ago
Merging #113 (cb1f325) into master (13e231a) will increase coverage by
9.34%
. The diff coverage is95.55%
.
@@ Coverage Diff @@
## master #113 +/- ##
==========================================
+ Coverage 39.30% 48.64% +9.34%
==========================================
Files 13 13
Lines 430 516 +86
==========================================
+ Hits 169 251 +82
- Misses 261 265 +4
Impacted Files | Coverage Δ | |
---|---|---|
src/distrs/tdist.jl | 100.00% <ø> (ø) |
|
src/distrs/binom.jl | 78.57% <62.50%> (-21.43%) |
:arrow_down: |
src/distrs/gamma.jl | 97.91% <97.61%> (-2.09%) |
:arrow_down: |
src/distrs/beta.jl | 100.00% <100.00%> (ø) |
|
src/distrs/chisq.jl | 100.00% <100.00%> (ø) |
|
src/distrs/fdist.jl | 100.00% <100.00%> (ø) |
|
src/distrs/pois.jl | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 13e231a...cb1f325. Read the comment docs.
I've added methods for Poisson via the definitions for gamma so this now supersedes #112. It requires https://github.com/JuliaRegistries/General/pull/37083 for the tests to pass so I'll restart once that PR is merged.
Let me butt in here to comment. Andreas has functions (pdf cdf ccdf logpdf logccdf invcdf invccdf invlogcdf invlogccdf) for Beta, Binomial, Chi Squared, F dist, Gamma, Normal, Poisson, and Student's T distribution.
A long ago PR in Distributions.jl has functions for Beta, Binomial, Cauchy, Chi Squared, F dist, Gamma, Geometric, HyperGeometric, logistic, lognormal, negative binomial, normal, poisson, student's t, uniform, weibull, and rayleigh distributions.
These pull requests always seem to stall. Review comments from devmotion tend to be along the lines of 1) small code optimizations, 2) questions on the "source" to certain algorithm constants. Multiply that by 8 for the all the distributions that Andreas is promoting. It results in a lot of little issues to fix/improve/modify. Review comments are valuable and it is leads to improved code, but one must manage the feedback process.
Perhaps it would be better to do a separate pull request for each distribution. It is more manageable to fix/modify one function at a time, rather than a set of 8. Learnings from fixing/modifying the first distribution would carry on to the second and subsequent distributions, so hopefully the process would be smoother. It is valuable to reference the code back to research papers; however, lacking that, we could depend on the test results matching the existing R functions.
Also, one might consider a parallel release strategy. Keep the R functions for now. Release a pure Julia version in parallel -- this is the "Beta test" -- let the community use it, study it, and identify problems (such as corner cases). After the Beta period, swap them for the R functions.
A long term goal has been to eliminate the dependence on the R functions. Don't let the perfect be the enemy of the good. These functions need to make it over the finish line.
I think it's fine. @devmotion comments here are very valid and should be addressed. The main complication is that some of the changes require adjustments in SpecialFunctions which makes it slightly more work but it's not that much. I had to look into some other things for a while but I'll get this completed.
Tried to fix support for betalogpdf, cdf can use a similar fix I think https://github.com/JuliaStats/StatsFuns.jl/pull/113/files#r663370467
Hello, I stumbled across this issue while benchmarking R vs Distributions.jl sampling. I think having pure julia implementations can be really useful to the ecosystem as a whole. I want to help out with this issue wherever I can; Is there anything that I can do?
PS: I have little experience in writing numerical software but am fluent in julia; given Rmath/similar implementations, I would be happy to port it to native julia :).
I've now rebased this one. There were many conflicts but they weren't too complicated so hopefully I've managed to avoid errors. I believe I've also addressed the comments that I promised to address (except the extra F distribution methods which I have locally). However, I'm hitting https://github.com/JuliaMath/SpecialFunctions.jl/pull/356 and it also looks like there is a type instability in the hypergeometric function.
I've pushed a lot of fixes. It was great with all the tests for values out of the support as it revealed many places that required more care. On top of https://github.com/JuliaMath/SpecialFunctions.jl/pull/374 and https://github.com/JuliaMath/SpecialFunctions.jl/pull/376 I now have all tests passing locally.
Yay! All tests pass now and I believe I have addressed the review comments so I think this one is ready. It only took me about eight months.
I theory we should probably backport the SpecialFunctions PRs since I consider all of them bug fixes.
@devmotion The fixes have been backported in https://github.com/JuliaRegistries/General/pull/54891 so are we good to go here?
I think we should begin to use Julia implementations when they are available. There is is really no need to call out to Rmath when we have implementations available. Eventually, we should then also be able to drop Rmath as a package dependency and just use it for testing although there is still some work to do. However, SpecialFunctions now has some of the more tricky functions available so we can get quite far. This has a conflict with https://github.com/JuliaStats/StatsFuns.jl/pull/112 so it would be good to get than one merged first and I can update this PR afterwards.