-
Notifications
You must be signed in to change notification settings - Fork 11
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
Update script.jl #79
Update script.jl #79
Conversation
Fixed some typos in the model and one in the code (variance was used instead of standard deviation in measurement model). Added PGAS sampling at the end to show that it solves the degeneracy problem, which should close the issue TuringLang#77
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #79 +/- ##
=======================================
Coverage 96.31% 96.32%
=======================================
Files 6 7 +1
Lines 380 381 +1
=======================================
+ Hits 366 367 +1
Misses 14 14 ☔ View full report in Codecov by Sentry. |
@@ -88,8 +86,8 @@ end | |||
|
|||
# Here we use the particle gibbs kernel without adaptive resampling. | |||
model = NonLinearTimeSeries(θ₀) | |||
pgas = AdvancedPS.PG(Nₚ, 1.0) | |||
chains = sample(rng, model, pgas, Nₛ; progress=false); |
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.
Minor comment, but why removing the rng
argument ? That makes the documentation reproducible (to some extent, rng implementations can change with julia versions)
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.
sorry, removed that when trying to get pgas to work. I edited the line now so that it is back to using rng as first argument, and also progress=false as in the original example.
The |
added back the rng argument when sampling with PG.
Does that mean that more complicated / arbitrary models aren't compatible with |
My understanding is the essentially arbitrary models are possible (I am currently working with a non-standard one and it works), but that you need to define the transition and observation densities as well as the initial distribution as done in my PR. |
I think this is where some confusion would come in. The way it is shown here, those functions define some distribution that is used, but don't specify how they are used. In the old example, they are explicitly used along with |
Note that the old example (using PG) is still there, I just added PGAS as a follow-up (as the old example ended rather disappointingly with degeneracy and an Issue asked for how to use PGAS here). |
The documentation is rather sparse I agree, thanks for the feedback I'll spend some time improving it. For the model part. We've only implemented For function (model::NonStandardModel)(rng::Random.AbstractRNG)
logpdf = simulation_steps(rng, ...)
Libtask.produce(logpdf)
end Hopefully that helps. If you have an example that's causing confusion, happy to look into it. |
Oh, I see. The question was about models outside the state-space class, non-Markovian models. Thanks for the clarification. That restriction of PGAS should be mentioned in the docs where PGAS is introduced. Let me know if I should change anything here. |
A small note, github does not deploy previews from forks. You can also work directly on the main repo and github would build a preview version of the PRs. |
A very small thing, but the rest looks fine to me. Thanks again for looking into this ! |
Co-authored-by: FredericWantiez <frederic.wantiez@gmail.com>
Fixed some typos in the model description (the maths) and one in the code (variance was used instead of standard deviation in measurement model). Added PGAS sampling at the end to show that it solves the degeneracy problem, which should close the issue #77
Please check my text 'To use this sampler we need to define the transition and observation densities as well as the initial distribution in the following way:' as I am not sure that this is needed only for PGAS (since it was not needed for PG).