Accumulators for linked VarInfo#1212
Conversation
Benchmark Report
Computer InformationBenchmark Results |
|
DynamicPPL.jl documentation for PR #1212 is available at: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1212 +/- ##
============================================
- Coverage 78.36% 77.22% -1.15%
============================================
Files 41 45 +4
Lines 3296 3574 +278
============================================
+ Hits 2583 2760 +177
- Misses 713 814 +101 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
penelopeysm
force-pushed
the
py/shadow2
branch
from
8e674cc to
621d177
Compare
penelopeysm
force-pushed
the
py/link
branch
2 times, most recently
from
f4e1f8d to
ae49a68
Compare
penelopeysm
force-pushed
the
py/shadow2
branch
from
621d177 to
f10dbeb
Compare
penelopeysm
force-pushed
the
py/link
branch
2 times, most recently
from
28ceb12 to
8066136
Compare
penelopeysm
force-pushed
the
py/link
branch
8 times, most recently
from
b12d1bf to
06d224c
Compare
penelopeysm
force-pushed
the
py/link
branch
2 times, most recently
from
faea67a to
144f4d5
Compare
| if acc1.f != acc2.f | ||
| throw(ArgumentError("Cannot combine VNTAccumulators with different functions")) | ||
| end |
There was a problem hiding this comment.
I don't really know if this is too strict. I'm worried about things like anonymous functions, which don't compare equal to one another. But at the same time if we don't check, we might silently get unexpected results! So I've just gone with a general principle of, it doesn't break CI, let's be strict, and if we find we need to not be strict we can change it later.
| # Note that we can't rely on the stored transform being correct (e.g. if new values | ||
| # were placed in `vi` via `unflatten!!`, so we regenerate the transforms. |
There was a problem hiding this comment.
this is the same as the behaviour of from_maybe_linked_internal_transform.
| x, init_logjac = with_logabsdet_jacobian(get_transform(tval), get_internal_value(tval)) | ||
| # TODO(penelopeysm): This could be inefficient if `tval` is already linked and | ||
| # `setindex_with_dist!!` tells it to create a new linked value again. In particular, | ||
| # this is inefficient if we use `InitFromParams` that provides linked values. The answer | ||
| # to this is to stop using setindex_with_dist!! and just use the TransformedValue | ||
| # accumulator. |
There was a problem hiding this comment.
This scenario actually can't happen right now (there's nothing that triggers this code path). I don't want to fix it in this PR though because it'd get too big.
| # easier to type infer, but if `transform` no longer exists, it might start to cause | ||
| # unnecessary type inconcreteness in the elements of PartialArray. | ||
| """ | ||
| TransformedValue{Linked,ValType,TransformType,SizeType} |
There was a problem hiding this comment.
Essentially, TransformedValue{false} is the same as VectorValue now, and TransformedValue{true} is the same as LinkedVectorValue. I generalised this so that we could accommodate the third subtype UntransformedValue as well.
|
I think this one should be easier to review :) I wrote a bit in the toplevel comment about what this PR does & where the code changes are. Again no rush, I have plenty of other things to work on 😅 |
sunxd3
left a comment
sunxd3
left a comment
There was a problem hiding this comment.
Thanks, Penny!
LGTM in general, couple of nits
src/transformed_values.jl
Outdated
| get_transform(tv::$T) = tv.transform | ||
| get_internal_value(tv::$T) = tv.val | ||
|
|
||
| function update_value(tv::$T, new_val) |
There was a problem hiding this comment.
do we need update_value for UntransformedValue?
There was a problem hiding this comment.
That's probably a good idea, it's not used anywhere, but just for the sake of having a consistent API.
Also it should probably be update_internal_value or maybe even set_internal_value
Co-authored-by: Xianda Sun <5433119+sunxd3@users.noreply.github.com>
|
Thanks @sunxd3! |
Release 0.40 --------- Co-authored-by: Penelope Yong <penelopeysm@gmail.com> Co-authored-by: Xianda Sun <5433119+sunxd3@users.noreply.github.com>
2 participants
This PR implements something like what I described here: #836 (comment)
TODO
Description
This PR:
modifies the signature of
DynamicPPL.init, to return anAbstractTransformedValue;passes the
AbstractTransformedValuethrough to the accumulators;uses an accumulator,
TransformedValueAccumulator, to potentially modify and store a newAbstractTransformedValue.The general concept is explained in these new docs pages:
https://turinglang.org/DynamicPPL.jl/previews/PR1212/flow/
https://turinglang.org/DynamicPPL.jl/previews/PR1212/values/
About a third of the changes are docs. There are a lot of source code changes, but most of them are just adapting existing functions to use the new interface. The interesting code changes are, in recommended order of reading:
src/transformed_values.jlthe definition ofAbstractTransformedValuesrc/accs/vnt.jl, a generic accumulator that stores things in a VNT. Right now we only use it for one thing; I thought I'd leave the generalisation for another PR.src/accs/transformed_values.jl, the accumulator that specifically stores transformed values in a VNTsrc/varinfo.jl, the new implementation of linking that uses said accumulatorBenchmarks
main is main (of course)
py/orig-vnt is where Markus got to (#1183)
breaking is the same as py/orig-vnt, plus shadow arrays plus some perf improvements (#1204)
py/link is this PR
Generating a linked VarInfo directly
This PR also adds new methods for
VarInfo(rng, model, link, init), for example,VarInfo(model, LinkAll())will immediately generate a linked VarInfo.I thought that this would definitely be faster than the roundabout method, but it seems to depend on the model in question, and I'm not entirely sure why.