-
Notifications
You must be signed in to change notification settings - Fork 230
Vec assume #281
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
Vec assume #281
Conversation
|
Note about using array of array v.s. mat result from N = 20
beta = [0.5, 0.5]
setchunksize(N*length(beta))
alg = HMC(1000, 0.2, 4)
# Test for vectorize UnivariateDistribution
@model vdemo() = begin
phi = Vector{Vector{Real}}(N)
phi ~ [Dirichlet(beta)]
end
t_vec = @elapsed res_vec = sample(vdemo(), alg)
@model vdemo() = begin
phi = Matrix(2,N)
phi ~ [Dirichlet(beta)]
end
t_vec_mat = @elapsed res_vec_mat = sample(vdemo(), alg)
@model vdemo() = begin
phi = Vector{Vector{Real}}(N)
for i = 1:N
phi[i] ~ Dirichlet(beta)
end
end
t_loop = @elapsed res = sample(vdemo(), alg)
println("Time for")
println(" Loop : $t_loop")
println(" Vec : $t_vec")
println(" Vec2 : $t_vec_mat")gives Time for
Loop : 9.823776041
Vec : 21.934951717
Vec2 : 6.140146129with Mat seems much faster (should be also related to my implementation of converting mat back to nested array) |
|
Note: |
|
Also there is a typo(bug) in their fake verctorization implementation. For this reason I will not include the test file in runtest.jl |
|
@xukai92 Do you want me to review this PR? Seems tests are currently broken. |
|
Seems there is a bug of the resuming MCMC feature which may cause numerical error very easily. I guess it might be the case that when a chain is resumed, I call the |
|
@yebai I fixed few bugs and improve the stability a bit. Could you have a review now plz? |
impressive speedup! I've started reviewing this PR and will add my comments soon. |
|
|
||
| rs | ||
| if isa(dist, UnivariateDistribution) || isa(dist, MatrixDistribution) | ||
| var = rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we should add a dimensionality assertion here:
@assert size(var) == size(rs)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the assertions makes sense to me!
| var[i] = rs[:,i] | ||
| end | ||
| elseif isa(var, Matrix) | ||
| var = rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dimensionality assertion?
| for i = 1:n | ||
| push!(vi, vns[i], rs[i], dist, 0) | ||
| end | ||
| var = rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dimensionality assertion?
| var[i] = rs[:,i] | ||
| end | ||
| elseif isa(var, Matrix) | ||
| var = rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dimensionality assertion?
| lp | ||
| end | ||
|
|
||
| # REVIEW: why do we put this piece of code here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the distributions package does not check d.p[x] > 0.0, so I override their definition here. It's probably better to submit a PR to Distributions package to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I created an issue (#285) for this.
|
@yebai Tests on Travis look fine now. But tests on Apprvor got the same problem before - freeze before running actual tests. I'm playing around the settings/scripts to see if I can resolve it. |
|
NOTE: I did a profiling on the vectorized version of LDA model and it shows >70% time is spent by |
|
@yebai Can you give me permissions on appveyor so that I can terminate or restart tests? I have to wait for each one to be terminated by the sever to try to fix the issue, which is a bit slow-responsed. |
|
Done - I've added you to appveyor permissions list. |
|
@yebai Appveyor actually freezes when executing |
|
On working of #255
I'm trying to vectorize the transformation as well (#276). I already done that for univariate and simplex.
TODOs