Improvements to @submodel in #309
#348
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
As stated elsewhere, I would like to get this merged into #309 before we make a new release since otherwise we'll be making two breaking releases just after another. |
phipsgabler
requested changes
Dec 8, 2021
yebai
approved these changes
Dec 8, 2021
Co-authored-by: Philipp Gabler <[email protected]>
phipsgabler
approved these changes
Dec 8, 2021
|
Doctests should be passing now, at which point I'll merge it into #309 . |
devmotion
reviewed
Dec 8, 2021
Comment on lines
+58
to
+65
| Valid expressions for `prefix=...` are: | ||
| - `prefix=false`: no prefix is used. | ||
| - `prefix=true`: _attempt_ to automatically determine the prefix from the left-hand side | ||
| `... = model` by first converting into a `VarName`, and then calling `Symbol` on this. | ||
| - `prefix="my prefix"`: prefix is taken to be the static string "my prefix". | ||
| - `prefix=expression`: `expression` is evaluated at runtime, resulting in | ||
| the prefix `Symbol(expression)`. Note that this also includes string-interpolation, | ||
| e.g. `prefix="x[\$i]"` as it requires runtime information. |
There was a problem hiding this comment.
I'm late to the party but I thought I'd add a comment anyway 😄
Isn't the amount of supported types (in particular the boolean special case) and different behaviours here potentially confusing for users? And does the documentation have to distinguish between the last two cases (strings - or symbols? - and expressions) - isn't a string also an expression 😛 ?
There was a problem hiding this comment.
Well, we don't have to make the distinction clear to the user. Then all they need to know is that:
Boolspecifies whether or not do prefix, and iftruewe'll try to do it automatically.- Otherwise, it's like standard Julia code.
The reason why I decided to make them distinct is that they will potentially have different performance implications, as noted later on. But maybe you're right that it's unnecessary to put in the initial part of the docstring, so I'm happy to just combine prefix="my prefix" and prefix=expression into a common note, just saying that it will be converted into Symbol(result of evaluating expression).
There was a problem hiding this comment.
Yes, I think this would improve the docstring.
Additionally, another approach (I think Hong mentioned it somewhere and probably it was discussed there?) could be to always try to set it automatically if it is not provided and force users to state e.g. "prefix=nothing" if they don't want a prefix - assuming that one basically always wants a prefix. Then one could avoid the Booleans and would just have to handle nothing.
There was a problem hiding this comment.
Cool! I'll make that change and push it to #309 .
Additionally, another approach (I think Hong mentioned it somewhere and probably it was discussed there?) could be to always try to set it automatically if it is not provided and force users to state e.g. "prefix=nothing" if they don't want a prefix - assuming that one basically always wants a prefix.
Though I do kind of like the nothing idea, I'm not a big fan of the automatic prefixing. It will error very often because you can capture any return-value on the LHS, and many of these ways, e.g. (a, b) = ..., is not compatible with VarInfo 😕
yebai
added a commit
that referenced
this pull request
Dec 12, 2021
* 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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a follow-up on the following discussion: https://github.com/TuringLang/DynamicPPL.jl/pull/309/files#r760070684.
I made a separate PR for this as I felt the discussion regarding this particular change was a bit hidden in all the other changes in that PR + it's not strictly required for #309 to go through.