-
Notifications
You must be signed in to change notification settings - Fork 227
Dispatch error on loglikelihood when sampling from arraydist of multivariate distributions #2549
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
Comments
This comes from the use of
On the other hand, assumed variables (i.e. latent parameters) call The cheapest fix would be to patch DistributionsAD to include
But more generally, I don't fully understand why we use |
I realised (feeling a little foolish) that it works fine when just entering x as a matrix instead of a vector of vectors. This works:
Maybe one could automate the transformation there, or have a nice error message that saves people like me from making this mistake (using a vector of vectors feels very intuitive). I closed the issue (in embarrassment), but saw that you responded with a somewhat more general point - feel free to reopen! And thanks for the fast response :) |
(The vector of vectors solution would also be more flexible, since it wouldn't require all the vectors to be the same length, so perhaps this would be nice to support after all) |
Sorry for the many messages: I just realised it of course should have been:
Without wrapping the matrix in a vector. Both works, however - and it surprises me quite a lot that the matrix-wrapped-in-vector option worked |
Glad you figured out how to get it to work. I don't think we'll add support for vectors of vectors. I'm not sure I, too, was surprised that the matrix wrapped in a vector worked. I suspect the answer is automatic broadcasting for loglikelihood.
I tried switching to julia> @model function f(vector, matrix)
mean ~ Normal()
vector ~ Normal(mean)
matrix ~ MvNormal(fill(mean, 2), I)
return nothing
end
julia> f([1.0, 2.0], [1.0 2.0 3.0; 4.0 5.0 6.0])
julia> logpdf(Normal(), [0.1, 0.2])
┌ Warning: `logpdf(d::UnivariateDistribution, X::AbstractArray{<:Real})` is deprecated, use `map(Base.Fix1(logpdf, d), X)` instead.
│ caller = top-level scope at REPL[23]:1
└ @ Core REPL[23]:1
2-element Vector{Float64}:
-0.9239385332046728
-0.9389385332046728
julia> loglikelihood(Normal(), [0.1, 0.2])
-1.8628770664093457 and we need the latter behaviour. This only comes up with |
Oh, yes, but I was thinking we should disallow it. |
I think we would be better off without it (since |
Minimal working example
Description
Hey Turing,
As always, a huge appreciation for the incredible software you are building!
I hope this is not a known issue or a duplicate!
I get this error:
MethodError: no method matching loglikelihood(::DistributionsAD.VectorOfMultivariate{Continuous, IsoNormal, Vector{…}}, ::Vector{Vector{…}})
when I sample from an arraydist of multivariate distributions (see model m1).
It works if I sample in a for loop (m2), and it also works when there is no data and it estimates x as parameters (m3). So I expect a fix shouldn't be too difficult, although I don't know the internals of Turing's calculation of loglikelihoods well enough to say for sure.
Let me know if I can do anything! The easy workaround for me is of course just to use the for loop for now.
Warm regards!
Julia version info
versioninfo()
Manifest
]st --manifest
The text was updated successfully, but these errors were encountered: