-
Notifications
You must be signed in to change notification settings - Fork 218
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
Problem with using predict
with vector valued random variables
#2239
Comments
Okay, so the fact that any of this works is not great 😅 A few immediate things:
The reason why the 2nd scenario works at all is because the prior distribution for In short, Turing.jl executes the model once before running the main part of the We have some checks in place to warn the user about these scenarios, but we clearly need more since this slipped through the cracks! Effectively, if you want variables to be treated as i.i.d. rather than as a single multivariate, then you should either use # Nevermind; this also doesn't work...
@model function mv_normal_3(n)
σ ~ truncated(Normal(0., 0.1), lower = 0.)
μ = Vector{eltype(σ)}(undef, n)
μ .~ Normal()
x ~ MvNormal(μ, σ) # noise
return x
end
mdl3_10 = mv_normal_3(10)
x_data3 = mdl3_10()
chn3 = sample(mdl3_10 | (x = x_data3,), NUTS(), 2_000)
forecast_mdl3 = mv_normal_3(20)
forecast_chn3 = predict(forecast_mdl3, chn3; include_all = true) EDIT: Nvm, |
Thanks for the detailed explanation! So it turns out that this only works for models which have a representation where the priors are the same as the posterior. TBF, this is actually a pretty large class of forecast models: discrete-time numerical solutions to SDEs and the finite dimensional distributions of a GP can be written this way (we were motivated by having a standard parameterisation of latent white noise). |
So to be clear, in the example where you fix part of an array here using the |
Yeah, I definitively see the use for this! But it's somewhat non-trivial to support, so it really comes down to whether we want the maintenance burden of the functionality vs. having the user do some manual labour, i.e. use a
Exactly. The most annoying aprt of all this (IMO), is the perf implications of using vectorized vs. for loop. It's technically possible to do something like if @performing_inference
x ~ MvNormal(...)
else
x = Vector(undef, 10)
for i in eachindex(x)
x[i] ~ Normal(...)
end
end but we don't have that implemented (related: TuringLang/DynamicPPL.jl#510). |
One simple approach that could also work is for the aformentioned code to be generated automatically through an @iid x ~ MvNormal(...) and then this just converts to the above code block under the hood. |
Right. And this would avoid issues with (say) |
Exactly:) |
Did anything happen on this? @seabbs and myself are trying to make something where you can fully compose a fairly large set of Since, prediction is quite important here it would be a handy feature to have, but otoh I'm not inclined to go through every single model definition and put in a "forecast mode" boolean switch. Given the known behaviour of |
Noting this also appears to be an issue with filldist(Normal(), n) which is Doc'd (i think) as idd so I assumed it would work initially. |
I've started adding (CDCgov/Rt-without-renewal#369 (comment)) a PredictContext (+ hacking on
This seems even more ideal than the macro approach as that would be easy for a non-expert user to miss. |
Tagging @yebai to get some thoughts. Specifically on something like:
|
@seabbs @SamuelBrand1, we would be happy to hear more of your thoughts on syntax design. I think it has to be robust and intuitive for long-term maintenance. EDIT:
If we fix Of course, we should provide a warning message to the user in all other unsupported cases discussed here. |
Yes both of these would be great and cover our use case. It would also be nice to make it easier to switch modes (i.e. they specify a mode for inference and everything else manually) if users find other edge cases they want to fix but we wouldn't need that for what we are doing (at least at the moment). |
This sounds good, but would there be a performance implication? I'm wondering about the upsides/downsides here. |
IMO this is more of a doc-issue, as there are definitively scenarios where you it makes sense to use
It's somewhat unclear to me how this would work, but maybe this is something we should discuss in DynamicPPL.jl :) |
My point was that
What are the steps forward from here? In principle happy to help out with a proposed fixed but likely to need guidance / its unclear how y'all manage community contributions. It would be really great for us to have this fixed centrally vs implementing modifications in our tooling to get this working. |
Ah I see discussion over on TuringLang/DynamicPPL.jl#510 so I guess I'll track that!
This resolution does seem out of the scope of that issues however. |
Hi everyone,
Problem
There seems to be a problem with using
predict
in conjunction with models that use vectorisation.Consider this fairly simple example:
We can generate a dataset by sampling from this model for (say)$n = 10$ . The forecasting problem is then sampling for $n_f
= 11,...,20$ (only information propagated forward is about variance of noise).
However, this fails as per below:
The failure mode here seems to be that the sample underlying random variables for$\epsilon_i,~ i = 11,...,20$ gets drawn across samples from from
chn
.Fix 1:
mapreduce
acrossforecast
callsSo if you instead loop over samples and run
forecast
for each sample, this seems to work:Fix 2: Non-vectorised sampling
Or you can modify the underlying model to not use calls to vectorised random variables (although IMO this is non-ideal).
Ideal situation
Obviously, it would be ideal if
predict
"just worked" with vectorised random variables. Given the failure mode of naive usage ofpredict
I'm assuming that this is a problem with how the random numbers are generated around here?The text was updated successfully, but these errors were encountered: