Use pairwise summation in loglikelihood
#1409
Conversation
|
It almost feels like we should change the Base method for |
|
I'm not sure, I think this could be a good idea. However, even if |
Codecov Report
@@ Coverage Diff @@
## master #1409 +/- ##
==========================================
- Coverage 83.81% 83.76% -0.05%
==========================================
Files 122 120 -2
Lines 6993 6960 -33
==========================================
- Hits 5861 5830 -31
+ Misses 1132 1130 -2
Continue to review full report at Codecov.
|
|
@andreasnoack would you like to wait for a potential change in base? While the implementation is more verbose, I still think it has an advantage even if |
| end | ||
| function loglikelihood(d::MatrixDistribution, X::AbstractArray{<:AbstractMatrix{<:Real}}) | ||
| return sum(x -> logpdf(d, x), X) |
There was a problem hiding this comment.
Doesn't this call already use pairwise summation?
There was a problem hiding this comment.
Hmm, I thought it doesn't but seems you're right (was it always the case or did it change at some point?):
julia> A = vcat([Float32(1E0)], fill(Float32(1E-8), 10^8));
julia> sum(identity, A)
1.9999989f0
julia> sum(x -> x, A)
1.9999989f0
julia> sum(Broadcast.instantiate(Broadcast.broadcasted(x -> x, A)))
1.9999989f0In any case, I'll close this issue in favour of #1391 which will generalize logpdf and loglikelihood.
There was a problem hiding this comment.
No idea. That said, I get slightly different results in some cases:
julia> x = vcat([Float32(1E0)], fill(Float32(1E-8), 10^8));
julia> sum(sin, x) - sum(sin.(x))
-1.41859055f-5
|
In any case, I'll close this issue in favour of #1391 which will generalize |
This PR changes the implementation of
loglikelihoodsuch that it uses pairwise summation. This approach is based on JuliaLang/julia#31020 and uses a fast path in recent Julia versions. It is already used in e.g. StatsBase successfully (xref: JuliaStats/StatsBase.jl#518).