Skip to content
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

Reapply #113 and prepare release of StatsFuns 1.0.0 #142

Merged
merged 4 commits into from
Apr 29, 2022

Conversation

devmotion
Copy link
Member

This PR is a cleaned and corrected version of #139. Copied from there:

This PR is a follow-up to #138 and reapplies #113. It updates the compat entries and tests the oldest supported Julia version + LTS to ensure that users don't end up with (major) regressions compared with StatsFuns 0.9.17 that uses Rmath instead of Julia implementations of these functions.

andreasnoack and others added 4 commits April 20, 2022 09:12
* Use Julia implementations for (log)pdfs. Adjust Rmath tests to use
testsets to give more informative errors.

* Use Julia implementations for most cdf-like functions of the Chisq
distribution.

* Change Beta distribution to be using native implementations.

* Switch binomial cdfs to use native implementations (but not the inverses)

* Use native implementations for most cdf like gamma functions

* Just use the gamma defitions for the chisq

* Use the beta definitions for binomial

* Add native implementions for Poisson's cdf-like functions

* Rely on SpecialFunctions promotion handling in beta functions

* Rely on gamma_inc_inv promotion in gammainv(c)cdf

* Apply suggestions from code review

* Add compat for HypergeometricFunctions

* Handle negative arguments in Poisson (log)(c)cdf

* Handle negative x in Gamma (log)(c)cdf

* Handle arguments outside support for Beta and Fdist (log)(c)cdf

* Fix Fdist quantile functions

* Handle arguments out of support in Binomial (log)(c)cdf and apply
the floor function to make the results correct for non-integer arguments

* Avoid accidental promotion to Float64 in fdist(log)(c)cdf

* Fix typo in "x" calculation for fdist(log)(c)cdf

* Drop redundant low tolerance tests for Beta(1000, 2). We are now
generally more accurate than Rmath so failure are because of Rmath.

* Handle some non-finite edge cases in gamma and binomial

* Only switch to log-version of betalogcdf when the result is tiny

* Handle the degenerate cases in beta(log)(c)cdf when alpha is zero

* Handle the degenerate case of the gamma distribution when k==0.
Also, only calculate the log(c)cdf based on the hypergeometric
function when p is zero or subnormal

* Address review comments

* Avoid rational return type in gamma(log)(c)cdf

* Handle the case when alpha and beta are zero in the betacdf
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2022

Codecov Report

Merging #142 (ff7c85d) into master (a93d6b2) will increase coverage by 7.61%.
The diff coverage is 95.55%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   48.98%   56.60%   +7.61%     
==========================================
  Files          13       13              
  Lines         445      530      +85     
==========================================
+ Hits          218      300      +82     
- Misses        227      230       +3     
Impacted Files Coverage Δ
src/distrs/tdist.jl 100.00% <ø> (ø)
src/distrs/binom.jl 78.57% <62.50%> (-21.43%) ⬇️
src/distrs/gamma.jl 97.91% <97.61%> (-2.09%) ⬇️
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%> (ø)
src/distrs/norm.jl 97.43% <0.00%> (+1.23%) ⬆️

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 a93d6b2...ff7c85d. Read the comment docs.

@devmotion
Copy link
Member Author

@nalimilan I did not remove the reexports for now since it seems there was no general agreement in favour of this change (at least I remember you preferred them to be kept).

@nalimilan
Copy link
Member

@nalimilan I did not remove the reexports for now since it seems there was no general agreement in favour of this change (at least I remember you preferred them to be kept).

Yeah my point was that if we drop IrrationalConstants and LogExpFunctions reexports, this package only contains distributions-related functions, so it would better be renamed e.g. DistributionsFuns. Maybe at some point we could split out these functions to a new package, and StatsFuns would become a meta package and/or be deprecated.

Before releasing 1.0, are we sure we are happy with all of the API? AFAICT it's been very stable so I guess it's fine?

@devmotion
Copy link
Member Author

Before releasing 1.0, are we sure we are happy with all of the API? AFAICT it's been very stable so I guess it's fine?

Yes, in my opinion it has been quite stable, I think the main consideration was just whether to drop the reexports or not.

In any case, we can always move to 2.0 if something comes up that is worth changing but breaking.

@andreasnoack
Copy link
Member

In any case, we can always move to 2.0 if something comes up that is worth changing but breaking.

If that is our thinking then there is there is little reason to release 1.0. IMO releasing 1.0 only makes sense if we expect to commit to the API for some time.

@devmotion
Copy link
Member Author

If that is our thinking then there is there is little reason to release 1.0.

Well, that's my general opinion when people mention that some package should be moved to 1.0.

For StatsFuns I think it's not likely that there are any breaking changes anytime soon but I don't think moving to 1.0 should mean that useful breaking changes should be discouraged completely. However, even though I'm generally a bit hesitant regarding these pushes towards 1.0, there are some advantages I think: It is possible to distinguish between bugfix and feature releases, and one can also drop older Julia versions in a non-breaking way while being able to backport fixes to older Julia versions if needed.

@nalimilan
Copy link
Member

It doesn't seem there are any problems with the API (and my request on Slack didn't prompt any comments), so I'd say it's fine to release 1.0 -- unless you think we should drop reexports.

@devmotion
Copy link
Member Author

I don't mind either way, I think it's OK to keep them (for now at least). Just a final check as a follow-up to our discussion above, are you happy with the PR @andreasnoack?

@andreasnoack
Copy link
Member

Yes. Let's move forward with this as it is.

@devmotion devmotion merged commit c8f50cf into master Apr 29, 2022
@nalimilan nalimilan deleted the dw/statsfuns_1_0 branch May 3, 2022 16:12
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.

4 participants