[Merged by Bors] - Introduction of SamplingContext: keeping it simple#259
[Merged by Bors] - Introduction of SamplingContext: keeping it simple#259torfjelde wants to merge 53 commits into
SamplingContext: keeping it simple#259Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
| end | ||
|
|
||
| # HACK: Type-piracy. Is this really the way to go? | ||
| AbstractPPL.getsym(::AbstractVector{<:VarName{sym}}) where {sym} = sym |
There was a problem hiding this comment.
From the other PR:
It's just convenient for the dot_tilde_* statements where we are working with vns::AbstractVector{<:VarName{sym}}. This way we can just use getsym as before instead of dispatching on this (which will inevitably lead to method ambiguity due to the ordering of args in tilde)
There was a problem hiding this comment.
I checked a bit more carefully now and I think it is only needed because of
Line 83 in 30c8345
VarName{sym} in which the indices are mapped together (but not an array of VarName{sym}s).
However, I guess we might want to change this in a different PR since this is not newly introduced in the PR and quite different from the rest of the PR.
There was a problem hiding this comment.
That is not what I had in mind actually; I was thinking of https://github.com/TuringLang/DynamicPPL.jl/blob/tor%2Fsampling-context-simple/src/context_implementations.jl#L359-L369
The getsym call here will fail since vn is now vn::AbstractArray{<:VarName}. The alternative is to extract it in the method signature, but this is somewhat annoying + can lead to method ambiguities since it's one of the latter arguments for the method.
There was a problem hiding this comment.
I was thinking of https://github.com/TuringLang/DynamicPPL.jl/blob/tor%2Fsampling-context-simple/src/context_implementations.jl#L359-L369
This is how I understood your explanation 🙂 To me it just seemed that we only end up with AbstractArray{<:VarName{sym}} there because of the methods linked above. It seems that we do not support things like [VarName{sym1}(...), VarName{sym2}(...)] currently so I assumed this was the only reason for the arrays there. But maybe this is not the only way to get them?
There was a problem hiding this comment.
Aaah, I reread your previous comment, looked up the line you're referring to and now I see what you're saying.
But I think this is the only way actually. And regarding the assumptions it makes, I also noted the same but then thought "Does it even make sense to have dot_tilde_* implementation for cases [VarName{sym1}, VarName{sym2}]? Nah." You got anything in mind?
There was a problem hiding this comment.
Though I do like your suggestion (if I understood correctly) of allowing VarName to contain a collection of indices. But that should prob go to APPL in a different PR.
Sooooo type-piracy while we wait for that or nah?
There was a problem hiding this comment.
Yeah, let's go with the type piracy. Avoiding the AbstractArray{<:VarName{sym}} and changing these functions seems like the "correct" approach, but I don't think it has to be fixed in this PR and it seems a bit unrelated to all other changes here, so let's go with the type piracy and fix it later (and possibly in AbstractPPL - doesn't it already allow multiple indices? At least Colon() is supported IIRC).
There was a problem hiding this comment.
Might be good to open an issue, here and/or in AbstractPPL, though.
There was a problem hiding this comment.
Add it here: TuringLang/AbstractPPL.jl#7. Maybe a there's a lattice structure or something to varnames, it kind of feels like it, then we'd have join/meet and the meet is the collection of indices.
And yes, in theory something like VarName{x}((([1, 3],),)) already works, but I don't know how stable it is wrt. subsumption. I think it's mentioned as disallowed in the docstring.
|
This good @devmotion ? |
|
Difficult to keep track off all the different iterations but it seems fine 🙂 Do you intend to address the |
Yeah sorry 😕 As I said, I got a bit carried a way in the previous PRs. I opened a new one because I'd like to keep the other one around (probably close it, but still useful to have lying about for when we think more about handling contexts properly). Really want a good way to deal with all the contexts + make it easy to introduce new ones, but I'll leave it for now.
I'll address in this one (i.e. now) 👍 |
|
bors try |
|
Yo why isn't bors doing anything? Is he on strike? |
|
bors r+ |
This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <hg344@cam.ac.uk>
|
Build failed: |
|
I would suggest you @devmotion @yebai do another review before we merge it (this is why I did |
|
Also, why are the tests failiing like that? Didn't we just add a mechanism for allowing the integration tests to fail if we're making a breaking release? o.O EDIT: Ah, I see. I guess empty intersection is a |
|
bors try |
devmotion
left a comment
There was a problem hiding this comment.
It looks good mostly, I just spotted some incorrect docstrings and some minor things that were not addressed or reverted after you had applied some additional fixes. I also think that it would be more natural (and shouldn't break anything or require additional changes) to fall back to tilde_observe! and dot_tilde_observe! instead of tilde_observe and dot_tilde_observe when the variable names and indices are neglected.
| # Need the `logp` value, so we cannot defer `acclogp!` to child-context, i.e. | ||
| # we have to intercept the call to `tilde_observe!`. |
There was a problem hiding this comment.
Not to be addressed in this PR but maybe it would make sense to return value + logp in the tilde pipeline, also from tilde_assume! and tilde_observe!?
There was a problem hiding this comment.
Maybe, but then we need to change the compiler + the logp wouldn't actually be used for anything in the model. But yeah, might still be worth just to make the tilde-pipeline a bit nicer.
| @@ -0,0 +1,123 @@ | |||
| # A collection of models for which the mean-of-means for the posterior should | |||
There was a problem hiding this comment.
Ideally we wouldn't duplicate these in DynamicPPL and Turing... Maybe it would make sense to even add example models for tests to DynamicPPL (similar to test utilities for kernels in https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/blob/master/src/test_utils.jl)?
There was a problem hiding this comment.
I 100% agree with this, but a bit uncertain of the route to take. Though I think what you've done in KernelFunctions.jl is interesting.
There was a problem hiding this comment.
Good idea to have some test models inside DynamicPPL, which can be then imported by Turing. Overall, I think we should gradually make sure tests in DynamicPPL are more self-contained.
There was a problem hiding this comment.
Since we now perform integration tests with Turing (that also handle breaking and non-breaking changes correctly), maybe we can remove the remaining Turing-dependent tests in https://github.com/TuringLang/DynamicPPL.jl/tree/master/test/turing or integrate them with the other tests after removing the Turing dependency. These tests are already isolated and also don't block breaking updates anymore.
There was a problem hiding this comment.
That sounds like a good idea 👍
But do we leave that for another PR?
There was a problem hiding this comment.
Yes, I wouldn't include any additional changes here.
There was a problem hiding this comment.
I just ran bors this merges into tor/tilde-simplification anyways, so if we indeed want to include the test-refactoring we can do that there (IMO we leave it for a subsequent PR which should be soon, since that will be very useful)
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
| @@ -0,0 +1,123 @@ | |||
| # A collection of models for which the mean-of-means for the posterior should | |||
There was a problem hiding this comment.
Since we now perform integration tests with Turing (that also handle breaking and non-breaking changes correctly), maybe we can remove the remaining Turing-dependent tests in https://github.com/TuringLang/DynamicPPL.jl/tree/master/test/turing or integrate them with the other tests after removing the Turing dependency. These tests are already isolated and also don't block breaking updates anymore.
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
|
bors r+ |
This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <hg344@cam.ac.uk>
|
Build failed: |
…ynamicPPL.jl into tor/sampling-context-simple
|
bors r+ |
This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <hg344@cam.ac.uk>
SamplingContext: keeping it simpleSamplingContext: keeping it simple
* initial stuff * moved benchmark folder and added README * unwrap distributions and varnames at model-level * removed _tilde and renamed tilde_assume and others * formatting * updated compiler for new tilde-methods * fixed calls to dot_assume * added sampling context and unwrap_childcontext * updated tilde methods * updated model call signature * updated compiler * formatting * added getsym for vectors * Update src/varname.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed some signatures for Model * fixed a method call * fixed method signatures * sort of fixed the matchingvalue functionality for model * formatting * removed redundant _tilde method * removed left-over acclogp! that should not be here anymore * export SamplingContext * use context instead of ctx to refer to contexts * formatting * use context instead of ctx for variables * use context instead of ctx to refer to contexts * Update src/compiler.jl Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * Update src/context_implementations.jl Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * added some whitespace to some docstrings * deprecated tilde and dot_tilde plus exported new versions * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * minor version bump * added impl of matchingvalue for contexts * reverted the change that makes assume always resample * removed the inds arguments from assume and dot_assume to stay non-breaking * Update src/context_implementations.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * added missing sampler arg to tilde_observe * added missing sampler argument in dot_tilde_observe * fixed order of arguments in some dot_assume calls * formatting * formatting * added missing sampler argument in tilde_observe for SamplingContext * added missing word in a docstring * updated submodel macro * removed unwrap_childcontext and related since its not needed for this PR * updated submodel macro * fixed evaluation implementations of dot_assume * updated pointwise_loglikelihoods and related * added proper tests for pointwise_loglikelihoods * updated DPPL tests to reflect recent changes * bump minor version since this will be breaking * formatting * formatting * renamed mean_of_mean_models used in tests * bumped dppl version in integration tests * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * fixed ambiguity error * Introduction of `SamplingContext`: keeping it simple (#259) This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <hg344@cam.ac.uk> * Update src/DynamicPPL.jl Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * added initial impl of SimpleVarInfo * remove unnecessary debug statements to be compat with Zygote * make reconstruct slightly more generic * added a couple of convenience constructors * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * small fix * return var_info from tilde-statements, allowing impl of immutable versions * allow usage of non-Ref types in SimpleVarInfo * update submodel-macro * formatting and docstring for submodel-macro * attempt at supporting implicit returns too * added a small comment * simplifed submodel macro a bit * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed typo * use bang-bang convention * updated PointwiseLikelihoodContext * fixed issue where we unnecessarily replace the return-statement * check subtype in the retval * formatting * fixed type-instability in retval check * introduced evaluate method for model * remove unnecessary type-requirement * make return-value check much nicer * removed redundant creation of anonymous function * dont use UnionAll in return_values * updated tests for submodel to reflect new syntax * moved to using BangBang-convention for most methods * remove SimpleVarInfo from this branch * added a comment * reverted submodel macro to use = rather than ~ * updated SimpleVarInfo impl * added a couple of missing deprecations * updated tests * updated implementations of logjoint and others * formatting * added eltype impl for SimpleVarInfo * formatting * fixed eltype for SimpleVarInfo * updated to work with master * changed the output structure a bit * forgot to include src * updated jmd files * added some docs * updated README * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <devmotion@users.noreply.github.com> Co-authored-by: Hong Ge <hg344@cam.ac.uk>
* fixed some signatures for Model * fixed a method call * fixed method signatures * sort of fixed the matchingvalue functionality for model * formatting * removed redundant _tilde method * removed left-over acclogp! that should not be here anymore * export SamplingContext * use context instead of ctx to refer to contexts * formatting * use context instead of ctx for variables * use context instead of ctx to refer to contexts * Update src/compiler.jl Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * Update src/context_implementations.jl Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * added some whitespace to some docstrings * deprecated tilde and dot_tilde plus exported new versions * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * minor version bump * added impl of matchingvalue for contexts * reverted the change that makes assume always resample * removed the inds arguments from assume and dot_assume to stay non-breaking * Update src/context_implementations.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * added missing sampler arg to tilde_observe * added missing sampler argument in dot_tilde_observe * fixed order of arguments in some dot_assume calls * formatting * formatting * added missing sampler argument in tilde_observe for SamplingContext * added missing word in a docstring * updated submodel macro * removed unwrap_childcontext and related since its not needed for this PR * updated submodel macro * fixed evaluation implementations of dot_assume * updated pointwise_loglikelihoods and related * added proper tests for pointwise_loglikelihoods * updated DPPL tests to reflect recent changes * bump minor version since this will be breaking * formatting * formatting * renamed mean_of_mean_models used in tests * bumped dppl version in integration tests * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * fixed ambiguity error * Introduction of `SamplingContext`: keeping it simple (#259) This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <hg344@cam.ac.uk> * Update src/DynamicPPL.jl Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * added initial impl of SimpleVarInfo * remove unnecessary debug statements to be compat with Zygote * make reconstruct slightly more generic * added a couple of convenience constructors * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * small fix * return var_info from tilde-statements, allowing impl of immutable versions * allow usage of non-Ref types in SimpleVarInfo * update submodel-macro * formatting and docstring for submodel-macro * attempt at supporting implicit returns too * added a small comment * simplifed submodel macro a bit * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed typo * use bang-bang convention * updated PointwiseLikelihoodContext * fixed issue where we unnecessarily replace the return-statement * check subtype in the retval * formatting * fixed type-instability in retval check * introduced evaluate method for model * remove unnecessary type-requirement * make return-value check much nicer * removed redundant creation of anonymous function * dont use UnionAll in return_values * updated tests for submodel to reflect new syntax * moved to using BangBang-convention for most methods * remove SimpleVarInfo from this branch * added a comment * reverted submodel macro to use = rather than ~ * updated SimpleVarInfo impl * added a couple of missing deprecations * updated tests * updated implementations of logjoint and others * formatting * added eltype impl for SimpleVarInfo * formatting * fixed eltype for SimpleVarInfo * implement setindex!! in prep for allowing sampling with immutable vi * formatting * initial work on allowing sampling using SimpleVarInfo * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * add constructor for SimpleVarInfo using model * improved leftover to_namedtuple_expr, fixing a bug when used with Zygote * bumped patch version * fixed set_flag!! * forgot the return in the replace_returns * bigboy update to benchmarks * fixed some issues and added support for usage of Dict in SimpleVarInfo * added docstring and improved indexing behvaior for SimpleVarInfo * formatting * dont allow sampling with indexing when using SimpleVarInfo with NamedTuple unless shapes are specified * _setval_kernel and others are only supported by VarInfo atm * fixed typo in comment * added more values_as impls * removed redundant values_from_metadata * fixed bug in push!! for SimpleVarInfo * forgot which branch Im on * added handling of short defs in replace_returns and more docstrings * fixed bug in generate_tilde introduced in a merge * fixed a bug in isfuncdef * fixed tests * formatting * uncomment mistakenly commented code * bumped version * updated doctests * dont carry over bang-bang versions that we dont need for general varinfos * Apply suggestions from @phipsgabler Co-authored-by: Philipp Gabler <phipsgabler@users.noreply.github.com> * updated tests * removed unnecessary BangBang methods * fixed zygote rule for dot_observe * fixed Setfield.jl + returning VarInfo bug in model-macro * updated tests * fixed docs * formatting * fixed issues when using ThreadSafeVarInfo * fixed _pointwise_observe for ThreadSafeVarInfo * updated ThreadSafeVarInfo * made SimpleVarInfo compat with ThreadSafeVarInfo and added show * added some tests for return-values of models * formatting * fixed doctest for SimpleVarInfo * formatting * removed comparison of show from doctest for SimpleVarInfo * Update src/compiler.jl Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * removed OrderedCollections from docs * some additional fixes * fixed method ambiguity and some ill-defined map * renamed evaluate to evaluate!! * added implementations of haskey, getindex and setindex!! for SimpleVarInfo * formatting * dropped redundant definition * use getproperty instead of getindex * fixed method-ambiguity and added some comments * fixed docstring of SimpleVarInfo * fixed docstrings * fixed Project.toml for docs * fixed docstring of canview * fixed docstrings * another attempt at fixing docstrings * added a TODO comment * remove some output from docstring of SimpleVarInfo * fixed haskey and hasvalue for AbstractDict * updated some comments * updated some errors * added sampling dot_assume for SimpleVarInfo * added true versions of density computations to TestUtils * added tests specific for SimpleVarInfo * also document TestUtils * added TestUtils to docs * fixed setindex!! for SimpleVarInfo using AbstractDict * added more tests * formatting * dont use BangBang for setall! * revert unnecessary changes to settrans! * revert unnecessary changes to set_flag! * revert some changes to docstrings * fixed some comments and docstrings * added more convenient logjoint, logprior, and loglikelihood methods * removed unnecessary export * fixed export * use the Setfield impl of getindex, etc. as default and specialize on AbstractDict * fixed docstrings of logjoint, etc. * Apply suggestions from code review Co-authored-by: Philipp Gabler <phipsgabler@users.noreply.github.com> * fixed docstring for model * replaced return_values by capturing return-value from tilde-statements instead * added some tests for return-value of model * added broadcast_foreach * Apply suggestions from @devmotion Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * remove broadcast_foreach for now * some fixes to ThreadSafeVarInfo * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * fixed docstrings * forgot qualification for set * formatting * added comment about why we cant use MacroTools.isdef * remove unnecessary deprecation * udpated some docstrings * fixed more docstrings * make overloads of BangBang methods qualified * remove overloading of values and instead use values_as without the type specified * Apply suggestions from code review Co-authored-by: David Widmann <devmotion@users.noreply.github.com> * renamed hasvalue for SimpleVarInfo to _haskey * revert changes from previous commit * minor version bump * fixed sampling with ThreadSafeVarInfo * fixed setindex!! for ThreadSafeVarInfo * fixed eltype for ThreadSafeVarInfo wrapping a SimpleVarInfo * fixed a test * relax atol in serialization tests a bit * temporarily disable Julia 1.3 * relax atol for a prior check * Improvements to `@submodel` in #309 (#348) * added prefix keyword argument to submodel-macro * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * converted example in docs into test * fixed docstring * Apply suggestions from code review Co-authored-by: Philipp Gabler <phipsgabler@users.noreply.github.com> * removed redundant prefix_submodel_context def and added another example to docstring * fixed doctests * attempt at fixing doctests * another attempt at fixing doctests * had a typo in docstring Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Philipp Gabler <phipsgabler@users.noreply.github.com> * fixed a test case using submodel * improved docstring according to comments by @devmotion Co-authored-by: David Widmann <devmotion@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Hong Ge <hg344@cam.ac.uk> Co-authored-by: Philipp Gabler <phipsgabler@users.noreply.github.com>
This is #253 but the only motivation here is to get
SamplingContextin, nothing relating to interactions with other contexts, etc.