-
Notifications
You must be signed in to change notification settings - Fork 32
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
[Merged by Bors] - Introduction of SamplingContext
: keeping it simple
#259
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
@@ -39,3 +39,6 @@ Possibly existing indices of `varname` are neglected. | |||
) where {s,missings,_F,_a,_T} | |||
return s in missings | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked a bit more carefully now and I think it is only needed because of
Line 83 in 30c8345
return VarName(vn, (vn.indexing..., Tuple(i))) |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to open an issue, here and/or in AbstractPPL, though.
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.
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 <[email protected]>
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 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea 👍
But do we leave that for another PR?
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.
Yes, I wouldn't include any additional changes here.
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.
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 <[email protected]>
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]> * Update src/context_implementations.jl Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * 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 <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * 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 <[email protected]> * Update src/DynamicPPL.jl Co-authored-by: David Widmann <[email protected]> * 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 <[email protected]> Co-authored-by: Hong Ge <[email protected]>
* 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 <[email protected]> * Update src/context_implementations.jl Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * 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 <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * 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 <[email protected]> * Update src/DynamicPPL.jl Co-authored-by: David Widmann <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * remove broadcast_foreach for now * some fixes to ThreadSafeVarInfo * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * fixed a test case using submodel * improved docstring according to comments by @devmotion Co-authored-by: David Widmann <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Philipp Gabler <[email protected]>
This is #253 but the only motivation here is to get
SamplingContext
in, nothing relating to interactions with other contexts, etc.