-
Notifications
You must be signed in to change notification settings - Fork 227
DynamicPPL 0.36 #2535
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
DynamicPPL 0.36 #2535
Conversation
2327e65
to
c699b8c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2535 +/- ##
==========================================
+ Coverage 84.05% 84.27% +0.21%
==========================================
Files 21 21
Lines 1455 1456 +1
==========================================
+ Hits 1223 1227 +4
+ Misses 232 229 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 14910188111Details
💛 - Coveralls |
54ea767
to
30333d7
Compare
8c5f782
to
17721a9
Compare
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.
This is great! Leaving it unapproved just because I want to confirm that nothing bad has happened to type stability and hence runtimes.
Might it also make sense to throw in some non-trivially lensed varnames in the test models of the "type stability"
and "Sampler call order"
testsets in test/mcmc/gibbs.jl
?
Co-authored-by: Markus Hauru <[email protected]>
Turing.jl documentation for PR #2535 is available at: |
This PR adds compatibility with DynamicPPL 0.36.
Given that there are some reasonably breaking changes in the way that submodels behave, I've decided to increment the minor version number for Turing.
Because submodels now also prefix VarNames in a 'proper' way, submodel variables now have non-identity lenses. In order to allow Gibbs to sample models with submodels, this PR also fixes #2403 by letting GibbsContext store a tuple of VarNames rather than just Symbols. Performance is compared below.
There's some nasty lack of reproducibility in HMCDA sampling, which is causing the failures in
test/mcmc/Inference
. I haven't yet been able to repro this locally. However, I would like to deal with this in a separate PR, as the failure isn't introduced by this PR (the last CI run on main was already failing).This PR supersedes the CompatHelper ones at #2501 #2532 #2533
Benchmarks
I'm not 100% convinced of these, because my benchmarks have returned wildly variable results upon being run multiple times / when I restart my Julia session. Sometimes when I run
sample(rng, model, spl, 10000)
it runs in < 1 second and sometimes it takes 4 seconds. (This is after running it multiple times in the same session, so it's not an issue of JIT compilation.) I think I would probably feel a little bit more reassured if somebody else could test them out.However, broadly speaking, I think it's safe to say that I don't have any evidence of performance getting worse. Another noteworthy point is that CI for
test/mcmc/gibbs
(on ubuntu-latest, 1 thread) for this PR ran in 1 hr 25 min, which is the same time it took for the same job onmain
. So it seems unlikely that there are any serious performance implications (good or bad).