Skip to content
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] - Perform invlinking in assume rather than implicitly in getindex #360

Closed
wants to merge 107 commits into from
Closed
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
107 commits
Select commit Hold shift + click to select a range
14594d6
performing linking in assume rather than implicitly in getindex
torfjelde Jan 7, 2022
0bc279f
added istrans to SimpleVarInfo
torfjelde Jan 7, 2022
81ee12e
Apply suggestions from code review
torfjelde Jan 7, 2022
d39f87d
added a comment
torfjelde Jan 7, 2022
23f34cc
bump patch version
torfjelde Jan 7, 2022
d3ec108
introduced settrans!!
torfjelde Jan 9, 2022
81782c9
added istrans(vi) and renamed all occurences of trans! to trans!!
torfjelde Jan 9, 2022
12bfb42
exclusively use settrans!! to set the istrans for SimpleVarInfo
torfjelde Jan 10, 2022
c2c2417
removed usage of deprecated method in turing tests
torfjelde Jan 10, 2022
2e2cb5c
added docstring to settrans!!
torfjelde Jan 13, 2022
f6c3fc4
include istrans flag in type of SimpleVarInfo instead
torfjelde Jan 27, 2022
d643a78
deprecated settrans! in favour of settrans!!
torfjelde Jan 27, 2022
3cab7d9
added some tests specifically for istrans
torfjelde Jan 27, 2022
8b870dc
formatting
torfjelde Jan 27, 2022
0b304db
fixed bugs for ThreadSafeVarInfo
torfjelde Jan 27, 2022
b146b11
additional constructor for SimpleVarInfo
torfjelde Jan 27, 2022
d170d92
Update src/DynamicPPL.jl
torfjelde Feb 9, 2022
f46183b
added ConstructionBase.jl as dep
torfjelde Feb 9, 2022
7a78eec
added constraint types and doctests
torfjelde Feb 9, 2022
70b3b70
added DocStringExtensions as a dep
torfjelde Feb 9, 2022
a03e8cf
formatting
torfjelde Feb 9, 2022
793c931
remove redundant maybe_link
torfjelde Feb 9, 2022
a9b12fd
fixed typo
torfjelde Feb 9, 2022
b1d7f9a
Merge branch 'master' into tor/link-improvements
yebai Feb 9, 2022
be98961
moved a docstring
torfjelde Feb 11, 2022
3e1588b
fixed bug in tets
torfjelde Feb 11, 2022
7697fce
Merge branch 'tor/link-improvements' of github.com:TuringLang/Dynamic…
torfjelde Feb 11, 2022
3139c62
version bump
torfjelde Feb 11, 2022
3610658
added missing istrans impl
torfjelde Feb 12, 2022
27171ad
fixed bug with istrans
torfjelde Feb 13, 2022
cd2d9d6
fixed issue with getindex_raw for VarInfo
torfjelde Feb 13, 2022
d948cb9
Update src/varinfo.jl
torfjelde Feb 13, 2022
6a3e18f
Merge branch 'master' into tor/link-improvements
torfjelde Jun 10, 2022
d674478
Merge branch 'master' into tor/link-improvements
torfjelde Jun 17, 2022
26d2dbb
getindex of varinfo implementations now optionally takes a Distributi…
torfjelde Jun 22, 2022
3fcba56
use get_index_raw with dist argument
torfjelde Jun 22, 2022
83a9448
added missing assume implementations for SimpleVarInfo
torfjelde Jun 22, 2022
356fa9c
fixed settrans!! for VarInfo
torfjelde Jun 22, 2022
13f037f
formatting
torfjelde Jun 24, 2022
c7544e0
fixed bug where constrained/unconstrained wasn't preserved in setinde…
torfjelde Jun 24, 2022
d1dccf1
hack to avoid type-instabilities for dot_assume with MultivariateDist…
torfjelde Jun 24, 2022
ff7ff4a
style
torfjelde Jun 26, 2022
2f1a2ff
added keys implementations for the models in TestUtils to make testin…
torfjelde Jun 26, 2022
d6311b7
added additional test model which uses dot-assume on MultivariateDist…
torfjelde Jun 26, 2022
ed2fa69
updated tests for SimpleVarInfo
torfjelde Jun 26, 2022
a82be56
added a no-op reconstruct for UnivariateDistribution
torfjelde Jun 26, 2022
7aacee5
fixed tests for loglikelihoods
torfjelde Jun 27, 2022
96f128f
fixed dot_tilde_assume for LikelihoodContext
torfjelde Jun 27, 2022
2e88d08
removed some now redundant explicit calls to maybe_invlink
torfjelde Jun 27, 2022
0f9765b
added impls of size and length for the wrapper distributions so they …
torfjelde Jun 27, 2022
116c95c
bumped version
torfjelde Jun 28, 2022
d797e99
removed redunant explict call to maybe_invlink
torfjelde Jun 28, 2022
44b2f66
added test model with array on RHS of a .~ statement
torfjelde Jun 29, 2022
81cd881
improved some of the default implementations of dot_assume
torfjelde Jun 29, 2022
2e14abd
removed unnecessary code in tests
torfjelde Jun 29, 2022
12adc83
improved linking usage in assumes for SimpleVarInfo
torfjelde Jun 29, 2022
af3e6ba
Merge branch 'master' into tor/link-improvements
yebai Jun 29, 2022
f7501df
added model for testing dynamic constraints
torfjelde Jun 30, 2022
abcabf4
added logjoint_true_with_logabsdet_jacobian to TestUtils
torfjelde Jun 30, 2022
fdee509
added test for dynamic constraints for SimpleVarInfo
torfjelde Jun 30, 2022
e974c83
fixed keys implementation of SimpleVarInfo
torfjelde Jun 30, 2022
6c6d5f5
reverted unintended change
torfjelde Jun 30, 2022
801bd4c
renamed Base.keys(model) to varnames(model) in TestUtils
torfjelde Jul 1, 2022
46f6f4c
added default implementation and docstring for TestUtils.varnames
torfjelde Jul 1, 2022
bcb767b
replace handwritten by DocStringExtensions
torfjelde Jul 1, 2022
c5be1c2
Apply suggestions from @devmotion
torfjelde Jul 1, 2022
f266929
Update src/context_implementations.jl
torfjelde Jul 1, 2022
c2dbbaf
removed some asserts and use broadcast instead of map
torfjelde Jul 1, 2022
1abb46c
replace map with broadcasting to ensure consistent behavior
torfjelde Jul 1, 2022
1086c6c
Update src/simple_varinfo.jl
torfjelde Jul 1, 2022
f2fb4a5
added a method nodist to allow broadcasting NoDist constructor
torfjelde Jul 1, 2022
490d24e
updated some tests
torfjelde Jul 1, 2022
6350ccd
renamed AbstractConstraint to AbstractTransformation and its subtypes
torfjelde Jul 1, 2022
951e4c3
updated tests
torfjelde Jul 1, 2022
dcd92c9
fixed nodist usage
torfjelde Jul 1, 2022
2922ffa
fixed implementation of nodist
torfjelde Jul 1, 2022
5266a4b
fixed typo
torfjelde Jul 1, 2022
3c38710
formatting
torfjelde Jul 1, 2022
ba92f3f
bump patch version
torfjelde Jul 1, 2022
70c864c
fixed ThreadsafeVarInfo
torfjelde Jul 1, 2022
66f41a9
Apply suggestions from code review
torfjelde Jul 1, 2022
eb2d6b5
allow type-stable settrans!! for SimpleVarInfo
torfjelde Jul 1, 2022
e8cdb91
use maybe_invlink in getindex for VarInfo
torfjelde Jul 1, 2022
359d384
added comment to warn about buggy behavior
torfjelde Jul 1, 2022
ab0a99b
Update src/context_implementations.jl
torfjelde Jul 1, 2022
dd10913
just fix potential bug in getindex for VarInfo
torfjelde Jul 1, 2022
18d28cc
revert previous change because it likely introduces bugs
torfjelde Jul 1, 2022
32b7aab
elaborate in comment regarding potential bug
torfjelde Jul 1, 2022
fb86231
Merge branch 'tor/link-improvements' of github.com:TuringLang/Dynamic…
torfjelde Jul 1, 2022
f782fe2
added error message to dot_assume
torfjelde Jul 1, 2022
7d3493d
added error message to dot_assume again
torfjelde Jul 1, 2022
f0f981b
added _protect_dists method to help with broadcasting of NoDist
torfjelde Jul 2, 2022
1e0b946
simplified show for SimpleVarInfo
torfjelde Jul 2, 2022
faa0e42
styling
torfjelde Jul 2, 2022
9e7f493
fixed bug in show for SimpleVarInfo
torfjelde Jul 3, 2022
0a9383b
Revert "added _protect_dists method to help with broadcasting of NoDist"
torfjelde Jul 3, 2022
d8b0a75
fixed getindex with vector of varnames for AbstractVarInfo
torfjelde Jul 3, 2022
400f90f
Improvements to TestUtils (follow-up from #360) (#415)
torfjelde Jul 20, 2022
9241acd
fixed tests for distribution_wrappers
torfjelde Jul 21, 2022
947e5c6
upper bound Distributions because tests are sooooo slow due to deprec…
torfjelde Jul 21, 2022
6f9be0d
Update bors.toml
yebai Jul 21, 2022
5c5b9ce
Revert "Update bors.toml"
torfjelde Jul 22, 2022
e0797cc
Revert "upper bound Distributions because tests are sooooo slow due t…
torfjelde Jul 22, 2022
4b0e0e1
switch of deprecation warnings from integration tests for now
torfjelde Jul 22, 2022
5a73c87
bump supported Julia version to 1.6
torfjelde Jul 23, 2022
bb43021
added ability to filter varnames to check in TestUtils.test_sampler
torfjelde Jul 23, 2022
d5a48f8
bump minor version
torfjelde Jul 23, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ AbstractPPL = "7a57a42e-76ec-4ea3-a279-07e840d6d9cf"
BangBang = "198e06fe-97b7-11e9-32a5-e1d131e6ad66"
Bijectors = "76274a88-744f-5084-9051-94815aaf08c4"
ChainRulesCore = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4"
ConstructionBase = "187b0558-2788-49d3-abe0-74a17ed4e7c9"
Distributions = "31c24e10-a181-5473-b8eb-7969acd0382f"
DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
MacroTools = "1914dd2f-81c6-5fcd-8719-6d5c9610ff09"
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
Expand All @@ -22,7 +24,9 @@ AbstractPPL = "0.3, 0.4"
BangBang = "0.3"
Bijectors = "0.5.2, 0.6, 0.7, 0.8, 0.9, 0.10"
ChainRulesCore = "0.9.7, 0.10, 1"
ConstructionBase = "1"
Distributions = "0.23.8, 0.24, 0.25"
DocStringExtensions = "0.8"
MacroTools = "0.5.6"
Setfield = "0.7.1, 0.8"
ZygoteRules = "0.2"
Expand Down
6 changes: 6 additions & 0 deletions src/DynamicPPL.jl
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ using Setfield: Setfield
using Setfield: Setfield
using BangBang: BangBang

using DocStringExtensions

using Random: Random

import Base:
Expand Down Expand Up @@ -176,4 +178,8 @@ include("test_utils.jl")
@deprecate acclogp!(vi, logp) acclogp!!(vi, logp)
@deprecate resetlogp!(vi) resetlogp!!(vi)

@deprecate settrans!(vi::AbstractVarInfo, trans::Bool, vn::VarName) settrans!!(
vi, trans, vn
) false

end # module
75 changes: 49 additions & 26 deletions src/context_implementations.jl
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ end
function tilde_assume(context::PriorContext{<:NamedTuple}, right, vn, vi)
if haskey(context.vars, getsym(vn))
vi = setindex!!(vi, vectorize(right, get(context.vars, vn)), vn)
settrans!(vi, false, vn)
settrans!!(vi, false, vn)
end
return tilde_assume(PriorContext(), right, vn, vi)
end
Expand All @@ -64,15 +64,15 @@ function tilde_assume(
)
if haskey(context.vars, getsym(vn))
vi = setindex!!(vi, vectorize(right, get(context.vars, vn)), vn)
settrans!(vi, false, vn)
settrans!!(vi, false, vn)
end
return tilde_assume(rng, PriorContext(), sampler, right, vn, vi)
end

function tilde_assume(context::LikelihoodContext{<:NamedTuple}, right, vn, vi)
if haskey(context.vars, getsym(vn))
vi = setindex!!(vi, vectorize(right, get(context.vars, vn)), vn)
settrans!(vi, false, vn)
settrans!!(vi, false, vn)
end
return tilde_assume(LikelihoodContext(), right, vn, vi)
end
Expand All @@ -86,7 +86,7 @@ function tilde_assume(
)
if haskey(context.vars, getsym(vn))
vi = setindex!!(vi, vectorize(right, get(context.vars, vn)), vn)
settrans!(vi, false, vn)
settrans!!(vi, false, vn)
end
return tilde_assume(rng, LikelihoodContext(), sampler, right, vn, vi)
end
Expand Down Expand Up @@ -194,7 +194,9 @@ end

# fallback without sampler
function assume(dist::Distribution, vn::VarName, vi)
r = vi[vn]
# x = vi[vn]
r_raw = getindex_raw(vi, vn)
r = maybe_invlink(vi, vn, dist, r_raw)
return r, Bijectors.logpdf_with_trans(dist, r, istrans(vi, vn)), vi
Copy link
Member

@yebai yebai Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'll be nice if we can overload the transform used inside logpdf_with_trans in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can actually replace the above with

logp = zero(eltype(vi))
if istrans(vi, vn)
    r, logjac = forward(bijector(dist))
    logp -= logjac
else
    r = r_raw
end

return r, logpdf(dist, r)

right now. But I think if we do this we should remove logpdf_with_trans everywhere, so I wanted to defer that to another PR.

Copy link
Member

@yebai yebai Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phipsgabler this is what I mentioned today. We can now replace logpdf_with_trans with ChangeOfVariables/LogDensityInterface API.

#342

end

Expand All @@ -211,16 +213,23 @@ function assume(
if sampler isa SampleFromUniform || is_flagged(vi, vn, "del")
unset_flag!(vi, vn, "del")
r = init(rng, dist, sampler)
vi[vn] = vectorize(dist, r)
settrans!(vi, false, vn)
vi[vn] = vectorize(dist, maybe_link(vi, vn, dist, r))
setorder!(vi, vn, get_num_produce(vi))
else
r = vi[vn]
# Otherwise we just extract it.
# r = vi[vn]
r_raw = getindex_raw(vi, vn)
r = maybe_invlink(vi, vn, dist, r_raw)
end
else
r = init(rng, dist, sampler)
push!!(vi, vn, r, dist, sampler)
settrans!(vi, false, vn)
if istrans(vi)
push!!(vi, vn, link(dist, r), dist, sampler)
# By default `push!!` sets the transformed flag to `false`.
settrans!!(vi, true, vn)
else
push!!(vi, vn, r, dist, sampler)
end
torfjelde marked this conversation as resolved.
Show resolved Hide resolved
end

return r, Bijectors.logpdf_with_trans(dist, r, istrans(vi, vn)), vi
Expand Down Expand Up @@ -286,7 +295,7 @@ function dot_tilde_assume(context::LikelihoodContext{<:NamedTuple}, right, left,
var = get(context.vars, vn)
_right, _left, _vns = unwrap_right_left_vns(right, var, vn)
set_val!(vi, _vns, _right, _left)
settrans!.(Ref(vi), false, _vns)
settrans!!.(Ref(vi), false, _vns)
torfjelde marked this conversation as resolved.
Show resolved Hide resolved
dot_tilde_assume(LikelihoodContext(), _right, _left, _vns, vi)
else
dot_tilde_assume(LikelihoodContext(), right, left, vn, vi)
Expand All @@ -305,7 +314,7 @@ function dot_tilde_assume(
var = get(context.vars, vn)
_right, _left, _vns = unwrap_right_left_vns(right, var, vn)
set_val!(vi, _vns, _right, _left)
settrans!.(Ref(vi), false, _vns)
settrans!!.(Ref(vi), false, _vns)
torfjelde marked this conversation as resolved.
Show resolved Hide resolved
dot_tilde_assume(rng, LikelihoodContext(), sampler, _right, _left, _vns, vi)
else
dot_tilde_assume(rng, LikelihoodContext(), sampler, right, left, vn, vi)
Expand All @@ -326,7 +335,7 @@ function dot_tilde_assume(context::PriorContext{<:NamedTuple}, right, left, vn,
var = get(context.vars, vn)
_right, _left, _vns = unwrap_right_left_vns(right, var, vn)
set_val!(vi, _vns, _right, _left)
settrans!.(Ref(vi), false, _vns)
settrans!!.(Ref(vi), false, _vns)
torfjelde marked this conversation as resolved.
Show resolved Hide resolved
dot_tilde_assume(PriorContext(), _right, _left, _vns, vi)
else
dot_tilde_assume(PriorContext(), right, left, vn, vi)
Expand All @@ -345,7 +354,7 @@ function dot_tilde_assume(
var = get(context.vars, vn)
_right, _left, _vns = unwrap_right_left_vns(right, var, vn)
set_val!(vi, _vns, _right, _left)
settrans!.(Ref(vi), false, _vns)
settrans!!.(Ref(vi), false, _vns)
torfjelde marked this conversation as resolved.
Show resolved Hide resolved
dot_tilde_assume(rng, PriorContext(), sampler, _right, _left, _vns, vi)
else
dot_tilde_assume(rng, PriorContext(), sampler, right, left, vn, vi)
Expand Down Expand Up @@ -390,7 +399,9 @@ function dot_assume(
# m .~ Normal()
#
# in which case `var` will have `undef` elements, even if `m` is present in `vi`.
r = vi[vns]
# r = vi[vns]
r_raw = getindex_raw(vi, vns)
r = maybe_invlink(vi, vn, dist, r_raw)
lp = sum(zip(vns, eachcol(r))) do (vn, ri)
return Bijectors.logpdf_with_trans(dist, ri, istrans(vi, vn))
end
Expand Down Expand Up @@ -423,7 +434,8 @@ function dot_assume(
# m .~ Normal()
#
# in which case `var` will have `undef` elements, even if `m` is present in `vi`.
r = reshape(vi[vec(vns)], size(vns))
r_raw = getindex_raw(vi, vec(vns))
r = reshape(maybe_invlink.(Ref(vi), vns, dists, r_raw), size(vns))
lp = sum(Bijectors.logpdf_with_trans.(dists, r, istrans(vi, vns[1])))
return r, lp, vi
end
Expand Down Expand Up @@ -462,19 +474,24 @@ function get_and_set_val!(
r = init(rng, dist, spl, n)
for i in 1:n
vn = vns[i]
vi[vn] = vectorize(dist, r[:, i])
settrans!(vi, false, vn)
vi[vn] = vectorize(dist, maybe_link(vi, vn, dist, r[:, i]))
setorder!(vi, vn, get_num_produce(vi))
end
else
r = vi[vns]
r_raw = getindex_raw(vi, vns)
r = maybe_invlink(vi, vns, dist, r_raw)
end
else
r = init(rng, dist, spl, n)
for i in 1:n
vn = vns[i]
push!!(vi, vn, r[:, i], dist, spl)
settrans!(vi, false, vn)
if istrans(vi)
push!!(vi, vn, Bijectors.link(dist, r[:, i]), dist, spl)
# `push!!` sets the trans-flag to `false` by default.
settrans!!(vi, true, vn)
else
push!!(vi, vn, r[:, i], dist, spl)
end
end
end
return r
Expand All @@ -496,12 +513,13 @@ function get_and_set_val!(
for i in eachindex(vns)
vn = vns[i]
dist = dists isa AbstractArray ? dists[i] : dists
vi[vn] = vectorize(dist, r[i])
settrans!(vi, false, vn)
vi[vn] = vectorize(dist, maybe_link(vi, vn, dist, r[i]))
setorder!(vi, vn, get_num_produce(vi))
end
else
r = reshape(vi[vec(vns)], size(vns))
# r = reshape(vi[vec(vns)], size(vns))
r_raw = getindex_raw(vi, vec(vns))
r = maybe_invlink.(Ref(vi), vns, dists, reshape(r_raw, size(vns)))
torfjelde marked this conversation as resolved.
Show resolved Hide resolved
end
else
f = (vn, dist) -> init(rng, dist, spl)
Expand All @@ -511,8 +529,13 @@ function get_and_set_val!(
# 1. Figure out the broadcast size and use a `foreach`.
# 2. Define an anonymous function which returns `nothing`, which
# we then broadcast. This will allocate a vector of `nothing` though.
push!!.(Ref(vi), vns, r, dists, Ref(spl))
settrans!.(Ref(vi), false, vns)
if istrans(vi)
push!!.(Ref(vi), vns, link.(Ref(vi), vns, dists, r), dists, Ref(spl))
# `push!!` sets the trans-flag to `false` by default.
settrans!!.(Ref(vi), true, vns)
else
push!!.(Ref(vi), vns, r, dists, Ref(spl))
torfjelde marked this conversation as resolved.
Show resolved Hide resolved
end
end
return r
end
Expand Down
Loading