-
Notifications
You must be signed in to change notification settings - Fork 6
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
Benchmarking expected_loglik
#82
Comments
Bigger allocations, but less of them (and only a constant number). Few large allocations is more efficient than lots of small ones, I suppose! |
It might be helpful to see how it scales with number of Gauss-Hermite points, 10 vs 100 vs 1000... |
I think a better approach (that seems to break AD with basically all backends though according to my experiments in Distributions) is sum(Broadcast.instantiate(Broadcast.broadcasted(op, args...))) which also uses pairwise summation and is fast in recent Julia versions (JuliaLang/julia#31020). |
I'm not familiar with this package but this sounds like it could (or maybe should) be fixed in Zygote or with a ChainRule definition? |
The really weird thing is that it seems to happen on a Matrix/Vector product... |
(Completely unrelated: You could use |
SciML noticed the same Zygote issue it seems: SciML/SciMLSensitivity.jl#511 (comment) |
Follow up: function expected_loglik_david(
gh::GaussHermite, y::AbstractVector, q_f::AbstractVector{<:Normal}, lik
)
xs, ws = gausshermite(gh.n_points)
return sum(Broadcast.instantiate(
Broadcast.broadcasted(q_f, y) do q, y
μ = mean(q)
σ = std(q)
sum(Broadcast.instantiate(
Broadcast.broadcasted(xs, ws) do x, w
f = sqrt2 * σ * x + μ
loglikelihood(lik(f), y) * w
end
))
end
)) * invsqrtπ
end And it definitely improves a lot! Yet the linear algebra is always the fastest # David solution is the middle one
[ Info: 10
165.688 μs (188 allocations: 44.08 KiB)
158.399 μs (88 allocations: 32.47 KiB)
154.454 μs (87 allocations: 48.55 KiB)
[ Info: 100
411.624 μs (998 allocations: 146.06 KiB)
351.009 μs (88 allocations: 32.47 KiB)
307.118 μs (89 allocations: 191.22 KiB)
[ Info: 500
1.505 ms (4598 allocations: 599.25 KiB)
1.206 ms (88 allocations: 32.47 KiB)
1.234 ms (89 allocations: 825.78 KiB)
[ Info: 1000
2.882 ms (9098 allocations: 1.14 MiB)
2.277 ms (88 allocations: 32.47 KiB)
2.132 ms (89 allocations: 1.58 MiB) |
Now regarding AD, with Julia 1.7, Zygote 0.6.32, everything seems to pass ## Checking differentiation
using Zygote
N = 100
gh = GaussHermite(100)
lik = BernoulliLikelihood()
y = rand(0:1, N)
μ = randn(N)
σ = rand(N)
for f in [expected_loglik, expected_loglik_david, expected_loglik_old]
g = only(Zygote.gradient(x->f(gh, y, Normal.(x, σ), lik), μ))
end |
It seems |
I'll make a PR to use |
So with 1.7 (maybe 1.6 too?), there is an issue with Zygote gradients and the
expec_loglik
function because of it hits aBLAS
function.I tried to rewrite it to make fewer allocations and here are the results:
So the old approach is faster but make bigger allocations, I actually don't know where all this allocations come from for the first approach, any clue?
The text was updated successfully, but these errors were encountered: