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.

Sign up for GitHub

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] - Fix initialization of parameters for algorithms that use SampleFromUniform #232

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Fix initialization of parameters for algorithms that use SampleFromUniform #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b44800a
Fix initialization of parameters for algorithms that use `SampleFromU…
devmotion Apr 16, 2021
e94c1f6
Bump version
devmotion Apr 16, 2021
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
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name = "DynamicPPL"
uuid = "366bfd00-2699-11ea-058f-f148b4cae6d8"
version = "0.10.15"
version = "0.10.16"

[deps]
AbstractMCMC = "80f14c24-f653-4e6a-9b94-39d6b0f70001"
Expand Down
10 changes: 9 additions & 1 deletion src/sampler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,15 @@ function AbstractMCMC.step(
initialize_parameters!(vi, kwargs[:init_params], spl)

# Update joint log probability.
model(rng, vi, _spl)
# TODO: fix properly by using sampler and evaluation contexts
# This is a quick fix for https://github.com/TuringLang/Turing.jl/issues/1588
# and https://github.com/TuringLang/Turing.jl/issues/1563
# to avoid that existing variables are resampled
if _spl isa SampleFromUniform
model(rng, vi, SampleFromPrior())
else
model(rng, vi, _spl)
end
Comment on lines +80 to +88
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to maybe remove the check if it's SampleFromUniform here:

DynamicPPL.jl/src/context_implementations.jl

Line 128 in d2678d5

if spl isa SampleFromUniform || is_flagged(vi, vn, "del")

And instead we just provide a "resample!" or something with similar behavior as set_and_resample!(vi, ())?

Copy link
Member

Choose a reason for hiding this comment

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

Or will it be difficult to understand the downstream effects this might have?

Copy link
Member

Choose a reason for hiding this comment

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

In that case we could even allow init_params to be a NamedTuple and use set_and_resample! instead of initialize_parameters!.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think probably it will break downstream code if the sampler logic is changed. That's one motivation for temporarily introducing this workaround. The second motivation is that actually on the contrary it has been discussed to resample always when using a sampler, i.e., instead of not sampling with SampleFromUniform one would rather sample always also with e.g. SampleFromPrior. IMO this seems reasonable, in particular when one uses separate sampling and evaluation contexts since then it would be much clearer when the variables are affected by rerunning the model (and one could still avoid resampling).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I get that; I just realized that what I mentioned above would also allow the use of NamedTuple as init_params which might be way more convenient for end-users. But maybe that's best resolved in a different PR once we have introduced the contexts you mentioned 👍

end

return initialstep(rng, model, spl, vi; kwargs...)
Expand Down
120 changes: 64 additions & 56 deletions test/sampler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,76 +32,84 @@
end
@testset "Initial parameters" begin
# dummy algorithm that just returns initial value and does not perform any sampling
struct OnlyInitAlg end
abstract type OnlyInitAlg end
struct OnlyInitAlgDefault <: OnlyInitAlg end
struct OnlyInitAlgUniform <: OnlyInitAlg end
function DynamicPPL.initialstep(
rng::Random.AbstractRNG,
model::Model,
::Sampler{OnlyInitAlg},
::Sampler{<:OnlyInitAlg},
vi::AbstractVarInfo;
kwargs...,
)
return vi, nothing
end
DynamicPPL.getspace(::Sampler{OnlyInitAlg}) = ()
DynamicPPL.getspace(::Sampler{<:OnlyInitAlg}) = ()

# model with one variable: initialization p = 0.2
@model function coinflip()
p ~ Beta(1, 1)
10 ~ Binomial(25, p)
end
model = coinflip()
sampler = Sampler(OnlyInitAlg())
lptrue = logpdf(Binomial(25, 0.2), 10)
chain = sample(model, sampler, 1; init_params = 0.2, progress = false)
@test chain[1].metadata.p.vals == [0.2]
@test getlogp(chain[1]) == lptrue
# initial samplers
DynamicPPL.initialsampler(::Sampler{OnlyInitAlgUniform}) = SampleFromUniform()
@test DynamicPPL.initialsampler(Sampler(OnlyInitAlgDefault())) == SampleFromPrior()

# parallel sampling
chains = sample(
model, sampler, MCMCThreads(), 1, 10;
init_params = 0.2, progress = false,
)
for c in chains
@test c[1].metadata.p.vals == [0.2]
@test getlogp(c[1]) == lptrue
end
for alg in (OnlyInitAlgDefault(), OnlyInitAlgUniform())
# model with one variable: initialization p = 0.2
@model function coinflip()
p ~ Beta(1, 1)
10 ~ Binomial(25, p)
end
model = coinflip()
sampler = Sampler(alg)
lptrue = logpdf(Binomial(25, 0.2), 10)
chain = sample(model, sampler, 1; init_params = 0.2, progress = false)
@test chain[1].metadata.p.vals == [0.2]
@test getlogp(chain[1]) == lptrue

# model with two variables: initialization s = 4, m = -1
@model function twovars()
s ~ InverseGamma(2, 3)
m ~ Normal(0, sqrt(s))
end
model = twovars()
lptrue = logpdf(InverseGamma(2, 3), 4) + logpdf(Normal(0, 2), -1)
chain = sample(model, sampler, 1; init_params = [4, -1], progress = false)
@test chain[1].metadata.s.vals == [4]
@test chain[1].metadata.m.vals == [-1]
@test getlogp(chain[1]) == lptrue
# parallel sampling
chains = sample(
model, sampler, MCMCThreads(), 1, 10;
init_params = 0.2, progress = false,
)
for c in chains
@test c[1].metadata.p.vals == [0.2]
@test getlogp(c[1]) == lptrue
end

# parallel sampling
chains = sample(
model, sampler, MCMCThreads(), 1, 10;
init_params = [4, -1], progress = false,
)
for c in chains
@test c[1].metadata.s.vals == [4]
@test c[1].metadata.m.vals == [-1]
@test getlogp(c[1]) == lptrue
end
# model with two variables: initialization s = 4, m = -1
@model function twovars()
s ~ InverseGamma(2, 3)
m ~ Normal(0, sqrt(s))
end
model = twovars()
lptrue = logpdf(InverseGamma(2, 3), 4) + logpdf(Normal(0, 2), -1)
chain = sample(model, sampler, 1; init_params = [4, -1], progress = false)
@test chain[1].metadata.s.vals == [4]
@test chain[1].metadata.m.vals == [-1]
@test getlogp(chain[1]) == lptrue

# parallel sampling
chains = sample(
model, sampler, MCMCThreads(), 1, 10;
init_params = [4, -1], progress = false,
)
for c in chains
@test c[1].metadata.s.vals == [4]
@test c[1].metadata.m.vals == [-1]
@test getlogp(c[1]) == lptrue
end

# set only m = -1
chain = sample(model, sampler, 1; init_params = [missing, -1], progress = false)
@test !ismissing(chain[1].metadata.s.vals[1])
@test chain[1].metadata.m.vals == [-1]
# set only m = -1
chain = sample(model, sampler, 1; init_params = [missing, -1], progress = false)
@test !ismissing(chain[1].metadata.s.vals[1])
@test chain[1].metadata.m.vals == [-1]

# parallel sampling
chains = sample(
model, sampler, MCMCThreads(), 1, 10;
init_params = [missing, -1], progress = false,
)
for c in chains
@test !ismissing(c[1].metadata.s.vals[1])
@test c[1].metadata.m.vals == [-1]
# parallel sampling
chains = sample(
model, sampler, MCMCThreads(), 1, 10;
init_params = [missing, -1], progress = false,
)
for c in chains
@test !ismissing(c[1].metadata.s.vals[1])
@test c[1].metadata.m.vals == [-1]
end
end
end
end