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

Add doc strings and explicit BigInt method for lstirling_asym. #36

Merged
merged 10 commits into from
Mar 15, 2018

Conversation

dmbates
Copy link
Contributor

@dmbates dmbates commented Mar 8, 2018

My purpose in creating an explicit BigInt method for lstirling_asym was to get full accuracy on small integer values of x from the Float64 and Float32 methods by evaluating log(factorial(x-1)) explicitly and storing these in an array. The only downside I can see to this is that the function will be a bit discontinuous at these small integer values (perhaps literally a bit or two of discontinuity).

This way the Stirling error function stirlerr(n) in #33 can be replaced by lstirling_asym(n + 1)

src/basicfuns.jl Outdated

Return `x * log(x)` for `x ≥ 0`. (Special case is `x = 0`.)
Copy link
Member

@simonbyrne simonbyrne Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

Return `x * log(x)` for `x ≥ 0`, handling `x = 0` as the limit

```jldoctest
julia> StatsFuns.xlogx(0)
0.0
```

@simonbyrne
Copy link
Member

I've made a few minor tweaks.

@@ -115,8 +173,11 @@ function _log1pmx_ker(x::Float64)
end


## logsumexp
"""
logsumexp(x::Real, y::Real)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably rename this logaddexp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look a bit further down. There is a method for x::AbstractArray{<:Real} for which sum as part of the name is appropriate.

src/misc.jl Outdated
@@ -43,7 +72,8 @@ function lstirling_asym(x::Float64)
end

function lstirling_asym(x::Float32)
t = 1f0/(x*x)
isinteger(x) && (0 < x ≤ length(lstirlingF32)) && return lstirlingF32[Int(x)]
t = inv/(abs2(x))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean inv(abs2(x))?

@@ -18,3 +18,13 @@ poispdf(λ::Real, x::Real) = exp(poislogpdf(λ, x))
poislogpdf(λ::T, x::T) where {T <: Real} = xlogy(x, λ) - λ - lgamma(x + 1)

poislogpdf(λ::Number, x::Number) = poislogpdf(promote(float(λ), x)...)

#=
function poislogpdf(λ::Union{Float32,Float64}, x::Union{Float64,Float32,Integer})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/misc.jl Outdated
function lstirling_asym end

# For BigInt use the exact calculation of factorial(x-1)
function lstirling_asym(x::BigInt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lgamma is fine everywhere

julia> norm([log(factorial(n)) - lgamma(n + 1) for n in big(1):big(500)])
0.0

Maybe better to make change the signature to BigFloat to match the other versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that.

@dmbates
Copy link
Contributor Author

dmbates commented Mar 9, 2018

I will add more tests as part of this PR. I just noticed that the author of the tvpack.jl source file didn't include a single test. If he had he would have discovered that there is a dependency on the Distributions package that is not satisfied.

@dmbates
Copy link
Contributor Author

dmbates commented Mar 9, 2018

I see now that src/tvpack.jl is not included by the file src/StatsFuns.jl. Is tvpack.jl just there so that it doesn't get lost? It does have the effect of lowering the coverage statistics considerably.

@simonbyrne
Copy link
Member

regarding the lstirling_asym, I'm not sure what we should do for smaller non-integer values (when the asymptotic error is too large).

The two most obvious choices seem to be the Lanczos' and Spouge's approximations. They are similar, but the former converges faster (and is used by Boost), whereas the latter can be computed to arbitrary precision (and is used by MPFR).

@dmbates
Copy link
Contributor Author

dmbates commented Mar 9, 2018

@simonbyrne My purpose in looking at lstirling_asym was because of Loader's technique of evaluating binomlogpdf as described in #33. For binomlogpdf and for poislogpdf evaluation is always at positive integers and it is reasonable to store a vector of values up to the point where the lstirling_asym can be assumed to be accurate.

Do you know of other places where lstirling_asym is used?

@simonbyrne
Copy link
Member

At the moment, no. But if we can define it for real values we can use it for the gamma and beta densities as well.

@simonbyrne
Copy link
Member

I've tweaked the Stirling docs slightly, but lets merge this and we can flesh out other changes later.

src/misc.jl Outdated

lgamma(x) ≈ x*log(x) - x + log(2π/x)/2 = log(x)*(x-0.5) + log2π/2 -x
```math
\log \Gamma(x) \approx x \log(x) - x + log(2π/x)/2 = \log(x)*(x-1/2) + \log(2\pi)/2 - x
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed one log -> \log

@dmbates dmbates merged commit 5321025 into master Mar 15, 2018
@dmbates dmbates deleted the lstirling branch March 15, 2018 15:23
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