-
Notifications
You must be signed in to change notification settings - Fork 35
Unify {untyped,typed}_{vector_,}varinfo
constructor functions
#879
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
Benchmark Report for Commit dbddb2fComputer Information
Benchmark Results
|
{Untyped,Typed}{Vector,}VarInfo
constructors{Untyped,Typed}{Vector,}VarInfo
'constructors'
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #879 +/- ##
===========================================
Coverage ? 84.82%
===========================================
Files ? 34
Lines ? 3849
Branches ? 0
===========================================
Hits ? 3265
Misses ? 584
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/varinfo.jl
Outdated
symbols have been observed. `VarInfo{<:NamedTuple}` is aliased `TypedVarInfo`. | ||
symbols have been observed. `VarInfo{<:NamedTuple}` is aliased `NTVarInfo`. |
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.
Unsurprisingly, the changes are pretty much restricted to this file. The rest of the changes are pretty much just updating VarInfo -> TypedVarInfo.
logp::Base.RefValue{Tlogp} | ||
num_produce::Base.RefValue{Int} | ||
end | ||
const VectorVarInfo = VarInfo{<:VarNamedVector} | ||
VarInfo(meta=Metadata()) = VarInfo(meta, Ref{LogProbType}(0.0), Ref(0)) |
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.
The idea going forward is that VarInfo
is purely a low-level constructor for the struct itself, i.e., it is used to construct the VarInfo object from its fields, rather than doing something to instantiate those fields. Thus, one should not expect to be using it except at the very lowest levels of DynamicPPL, e.g. if one is testing basic properties of VarInfo objects.
src/varinfo.jl
Outdated
function UntypedVarInfo( | ||
model::Model, | ||
sampler::AbstractSampler=SampleFromPrior(), | ||
context::AbstractContext=DefaultContext(), | ||
) | ||
# No rng | ||
return UntypedVarInfo(Random.default_rng(), model, sampler, context) | ||
end | ||
function UntypedVarInfo(rng::Random.AbstractRNG, model::Model, context::AbstractContext) | ||
# No sampler | ||
return UntypedVarInfo(rng, model, SampleFromPrior(), context) | ||
end | ||
function UntypedVarInfo(model::Model, context::AbstractContext) | ||
# No sampler, no rng | ||
return UntypedVarInfo(model, SampleFromPrior(), context) | ||
end |
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 kinda dislike that i had to copy-paste this 4 times, once for each function. But I don't see a better way around it beyond doing indiscriminate f(args...) = f(g(args...))
.
We could make it quite a bit simpler by forbidding f(rng, model, context)
and f(model, context)
, i.e., if we want to specify a context then you also have to specify a sampler.
src/varinfo.jl
Outdated
""" | ||
function untyped_varinfo( | ||
function TypedVarInfo(vi::UntypedVarInfo) |
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 all shifted from lower in the file, it's not new code
I need to do a proper review, but some thoughts and questions based on just the description of changes: If The Vector versions are not exported, which I think is good. I view UntypedVectorVarInfo as the king-in-waiting, that will replace UntypedVarInfo once #881 is done. I also dream of a future where all VarInfos are typed, which we could handle using BangBang functions: If new elements need to be added to the NamedTuple we do so by creating a new object, otherwise we operate in-place. In that case I think we would have plain VarInfo be the only user-facing constructor. Do others have thoughts on the future external interface of this stuff? |
Yup - TypedVarInfo and TypedVectorVarInfo aren't types. I'm not opposed to making them lowercase but I guess for consistency everything should be lowercased so (I'd actually prefer if it was About interface: part of the thing I had in mind with this PR was to keep the interface relatively easy to change in the future. For example replacing MD with VNV is quite straightforward and basically means deleting the existing However, if we think that ultimately we just have a single |
I think if this was the final word on VarInfo, we should make the function names lowercase, because it's a pretty clear violation of Julia style conventions. However, I can see an argument for keeping them as they are to have less disruption if/when we change these again.
This sounds fair to me.
I like this idea, assuming others are onboard with my vision of a future with only a single |
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 like this, way more consistent. Some tiny details to note, and the above questions of whether to capitalise and/or deprecate. I could be happy keeping those as they are, just want to make sure we make a conscious decision.
src/varinfo.jl
Outdated
TypedVarInfo([rng, ]model[, sampler, context, metadata]) | ||
|
||
Return an untyped varinfo object for the given `model` and `context`. | ||
Return a VarInfo object for the given `model` and `context`, which has a NamedTuple of | ||
`Metadata` structs as its metadata field. | ||
|
||
# Arguments | ||
- `model::Model`: The model for which to create the varinfo object. | ||
- `context::AbstractContext`: The context in which to evaluate the model. Default: `SamplingContext()`. | ||
- `metadata::Union{Metadata,VarNamedVector}`: The metadata to use for the varinfo object. | ||
Default: `Metadata()`. | ||
- `rng::Random.AbstractRNG`: The random number generator to use during model evaluation | ||
- `model::Model`: The model for which to create the varinfo object | ||
- `sampler::AbstractSampler`: The sampler to use for the model. Defaults to `SampleFromPrior()`. | ||
- `context::AbstractContext`: The context in which to evaluate the model. Defaults to `DefaultContext()`. |
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.
Is there a reason to have this docstring here, rather than attached to the method it documents?
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, probably just me still trying to figure out exactly when Documenter works and exactly when it doesn't. Changed now
@mhauru What do you think of this overall approach?
|
I think I'm happy with that. I hesitated with the middle point, but I guess the lowercase versions weren't exported before either. Is the idea to avoid breakage if we one day move to only having one VarInfo type? |
Exactly (or in the case where people relied on VarInfo returning, specifically, a TypedVarInfo, at least we warned them). |
{Untyped,Typed}{Vector,}VarInfo
'constructors'{untyped,typed}_{vector_,}varinfo
constructor functions
c8f9073
to
04a2009
Compare
Bonus: The diff is much more readable now that |
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 @penelopeysm!
* 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>
Partly solves #862 (There's still the SimpleVarInfos; but this PR is big enough).
Why
There are a couple of issues that this PR attempts to solve.
First is the ambiguous meaning of
TypedVarInfo
. Its literal definition is:DynamicPPL.jl/src/varinfo.jl
Line 99 in eed80e5
This was fine in a pre-VarNamedVector world, because
UntypedVarInfo
meant VarInfo with Metadata, andTypedVarInfo
(implicitly) meant VarInfo with NamedTuple of Metadata. However, with VNV now on board, a VarInfo with a NT of VNV also falls underTypedVarInfo
. This means that sometimesTypedVarInfo
means "VarInfo with NT of anything" (paricularly when used in type signatures), but sometimes it still means "VarInfo with NT of Metadata" (see e.g. the similarly namedtyped_varinfo
function).Second is the inconsistent interface between different VarInfo constructors. We used to have
VarInfo(rng, model, ...)
as a very convenient way of constructing a TypedVarInfo (read: a VarInfo with NT of Metadata). None of the other possibilities had equally convenient methods.What
This PR therefore does the following:
untyped_varinfo([rng, ]model[, sampler, context])
--> VarInfo with Metadatatyped_varinfo([rng, ]model[, sampler, context])
--> VarInfo with NT of Metadatauntyped_vector_varinfo([rng, ]model[, sampler, context])
--> VarInfo with VNVtyped_vector_varinfo([rng, ]model[, sampler, context])
--> VarInfo with NT of VNVNote that all four functions are implemented such that if the rng is the same, the values will be the same, regardless of which type of VarInfo is requested. None of these functions are exported.
The
metadata
argument inVarInfo(rng, model, sampler, context, metadata)
no longer works. It was basically being used to specify whether it should be a Metadata or a VarNamedVector inside. If you wanted Metadata (the default) then useDynamicPPL.{untyped,typed}_varinfo
; if you wanted VarNamedVector then useDynamicPPL.{untyped,typed}_vector_varinfo
.TypedVarInfo
is no longer a type, it has been renamed toNTVarInfo
. It is also no longer a function (usetyped_varinfo
instead).The type
VectorVarInfo = VarInfo{<:VarNamedVector}
has been renamed toUntypedVectorVarInfo
for consistency. This was not exported (and still isn't).