-
Notifications
You must be signed in to change notification settings - Fork 12
Allow passing kwargs on to Libtask (when it's an AbstractTuringLibtaskModel...)
#118
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
Conversation
|
Yes, we'd like to merge the AdvancedPS code to Turing.jl. Libtask was holding us back; now, it is ready. |
ab20d4d to
fd8fa3f
Compare
|
AdvancedPS.jl documentation for PR #118 is available at: |
cf3da3f to
28c9e2e
Compare
28c9e2e to
051012c
Compare
|
Hi @yebai, sorry just tidying this up before I go away :) I am happy to try to work on upstreaming the necessary parts to Turing next year, but that might take more time, and this PR will unblock keyword arguments on SMC/PG on Turing (TuringLang/Turing.jl#2660). So I think it makes sense to merge this first. I tested locally and it all works as intended, so happy for it to be reviewed now (cc also @mhauru). |
AbstractTuringLibtaskModel...)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
=======================================
Coverage ? 95.41%
=======================================
Files ? 8
Lines ? 436
Branches ? 0
=======================================
Hits ? 416
Misses ? 20
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sunxd3
left a comment
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 logic looks good.
I am a bit worrying about introducing a struct with "Turing" in the naming (AbstractTuringLibtaskModel) into the namespace. This is purely aesthetic, of course. That said, not a blocker at all since we agree to move these into Turing pretty soon.
Thanks for the work, Penny!
This PR modifies LibtaskExt to allow sampling from Turing models with keyword arguments with SMC or PG. See TuringLang/Turing.jl#2660 for usage.
It's obviously quite ugly, the problem is that even though Turing is the only library that subtypes the abstract type, the abstract type has to be defined in AdvancedPS so that the LibtaskExt can dispatch on it. Ideally, that abstract type + any relevant methods inside LibtaskExt should just be shifted into Turing proper. That's for another time.