-
Notifications
You must be signed in to change notification settings - Fork 35
Fix condition
and fix
in submodels
#892
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #892 +/- ##
============================================
+ Coverage 84.76% 85.04% +0.28%
============================================
Files 35 35
Lines 3879 3919 +40
============================================
+ Hits 3288 3333 +45
+ Misses 591 586 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I'm silly, I deleted the test suite, of course it passed |
Benchmark Report for Commit fcb44e5Computer Information
Benchmark Results
|
Submodels now actually run faster. I was prepared to take a slight hit on performance, but this has exceeded my expectations. |
44d75fe
to
478c879
Compare
8abd633
to
fd0ee1d
Compare
8421c72
to
565f077
Compare
@yebai Oh, I like the newsletter board idea! |
Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com>
f67d179
to
8c3bff4
Compare
f7884f1
to
2d13f48
Compare
condition
and fix
in submodelscondition
, fix
, and variable prefixing in submodels
4dd93b5
to
8c3bff4
Compare
condition
, fix
, and variable prefixing in submodelscondition
and fix
in submodels
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.
Looks really good, thanks for doing this. I only had tiny comments and questions.
In Turing.jl, GibbsContext might benefit from a similar treatment. Or replacing it with use of ConditionContext. There was some reason why we didn't do that originally, I forget now what it was.
Co-authored-by: Markus Hauru <markus@mhauru.org>
I have never taken a look at GibbsContext, but I reckon I can do so after this DPPL release. |
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.
Thanks!
* Release 0.36 * AbstractPPL 0.11 + change prefixing behaviour (#830) * AbstractPPL 0.11; change prefixing behaviour * Use DynamicPPL.prefix rather than overloading * Remove VarInfo(VarInfo, params) (#870) * Unify `{untyped,typed}_{vector_,}varinfo` constructor functions (#879) * Unify {Untyped,Typed}{Vector,}VarInfo constructors * Update invocations * NTVarInfo * Fix tests * More fixes * Fixes * Fixes * Fixes * Use lowercase functions, don't deprecate VarInfo * Rewrite VarInfo docstring * Fix methods * Fix methods (really) * Link varinfo by default in AD testing utilities; make test suite run on linked varinfos (#890) * Link VarInfo by default * Tweak interface * Fix tests * Fix interface so that callers can inspect results * Document * Fix tests * Fix changelog * Test linked varinfos Closes #891 * Fix docstring + use AbstractFloat * Fix `condition` and `fix` in submodels (#892) * Fix conditioning in submodels * Simplify contextual_isassumption * Add documentation * Fix some tests * Add tests; fix a bunch of nested submodel issues * Fix fix as well * Fix doctests * Add unit tests for new functions * Add changelog entry * Update changelog Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> * Finish docs * Add a test for conditioning submodel via arguments * Clean new tests up a bit * Fix for VarNames with non-identity lenses * Apply suggestions from code review Co-authored-by: Markus Hauru <markus@mhauru.org> * Apply suggestions from code review * Make PrefixContext contain a varname rather than symbol (#896) --------- Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> Co-authored-by: Markus Hauru <markus@mhauru.org> --------- Co-authored-by: Markus Hauru <mhauru@turing.ac.uk> Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> Co-authored-by: Markus Hauru <markus@mhauru.org>
* Release 0.36 * AbstractPPL 0.11 + change prefixing behaviour (#830) * AbstractPPL 0.11; change prefixing behaviour * Use DynamicPPL.prefix rather than overloading * Remove VarInfo(VarInfo, params) (#870) * Unify `{untyped,typed}_{vector_,}varinfo` constructor functions (#879) * Unify {Untyped,Typed}{Vector,}VarInfo constructors * Update invocations * NTVarInfo * Fix tests * More fixes * Fixes * Fixes * Fixes * Use lowercase functions, don't deprecate VarInfo * Rewrite VarInfo docstring * Fix methods * Fix methods (really) * Draft of accumulators * Fix some variable names * Fix pointwise_logdensities, gut tilde_observe, remove resetlogp!! * Map rather than broadcast Co-authored-by: Tor Erlend Fjelde <tor.erlend95@gmail.com> * Start documenting accumulators * Use Val{symbols} instead of AccTypes to index * More documentation for accumulators * Link varinfo by default in AD testing utilities; make test suite run on linked varinfos (#890) * Link VarInfo by default * Tweak interface * Fix tests * Fix interface so that callers can inspect results * Document * Fix tests * Fix changelog * Test linked varinfos Closes #891 * Fix docstring + use AbstractFloat * Fix resetlogp!! and type stability for accumulators * Fix type rigidity of LogProbs and NumProduce * Fix uses of getlogp and other assorted issues * setaccs!! nicer interface and logdensity function fixes * Revert back to calling the macro @addlogprob! * Remove a dead test * Clarify a comment * Implement split/combine for PointwiseLogdensityAccumulator * Switch ThreadSafeVarInfo.accs_by_thread to be a tuple * Fix `condition` and `fix` in submodels (#892) * Fix conditioning in submodels * Simplify contextual_isassumption * Add documentation * Fix some tests * Add tests; fix a bunch of nested submodel issues * Fix fix as well * Fix doctests * Add unit tests for new functions * Add changelog entry * Update changelog Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> * Finish docs * Add a test for conditioning submodel via arguments * Clean new tests up a bit * Fix for VarNames with non-identity lenses * Apply suggestions from code review Co-authored-by: Markus Hauru <markus@mhauru.org> * Apply suggestions from code review * Make PrefixContext contain a varname rather than symbol (#896) --------- Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> Co-authored-by: Markus Hauru <markus@mhauru.org> * Revert ThreadSafeVarInfo back to Vectors and fix some AD type casting in (Simple)VarInfo * Improve accumulator docs * Add test/accumulators.jl * Docs fixes * Various small fixes * Make DynamicTransformation not use accumulators other than LogPrior * Fix variable order and name of map_accumulator!! * Typo fixing * Small improvement to ThreadSafeVarInfo * Fix demo_dot_assume_observe_submodel prefixing * Typo fixing * Miscellaneous small fixes * HISTORY entry and more miscellanea * Add more tests for accumulators * Improve accumulators docstrings * Fix a typo * Expand HISTORY entry * Add accumulators to API docs * Remove unexported functions from API docs * Add NamedTuple methods for get/set/acclogp * Fix setlogp!! with single scalar to error * Export AbstractAccumulator, fix a docs typo * Apply suggestions from code review Co-authored-by: Penelope Yong <penelopeysm@gmail.com> * Rename LogPrior -> LogPriorAccumulator, and Likelihood and NumProduce * Type bound log prob accumulators with T<:Real * Add @addlogprior! and @addloglikelihood! * Apply suggestions from code review Co-authored-by: Penelope Yong <penelopeysm@gmail.com> * Move default accumulators to default_accumulators.jl * Fix some tests * Introduce default_accumulators() * Go back to only having @addlogprob! * Fix tilde_observe!! prefixing * Fix default_accumulators internal type * Make unflatten more type stable, and add a test for it * Always print all benchmark results * Move NumProduce VI functions to abstract_varinfo.jl --------- Co-authored-by: Penelope Yong <penelopeysm@gmail.com> Co-authored-by: Tor Erlend Fjelde <tor.erlend95@gmail.com> Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com>
Summary
This PR closes #857, i.e., gives users sensible ways of conditioning and fixing variables in submodels. To aid in understanding this, it also adds a long piece of documentation (PREVIEW HERE) explaining the design.
Performance
In the table below I compare the time taken for
_evaluate!!(model, ...)
onbreaking
(the base branch) andpy/submodel-cond
(HEAD of this PR). For good measure, the followup PR #896 is also benchmarked.The model tested comprises
m
submodels, all of which containn
assumed variables. All times are in µs.Profiling code (click to expand)
TODO
fix
decondition
andunfix
is still sensible (it should be, but you never know...)inargnames
checks, only thecontextual_isassumption
checks; but probably good to test anyway)