-
Notifications
You must be signed in to change notification settings - Fork 219
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
Compatibility with new DPPL version #1900
Changes from 31 commits
b3973a0
b377ea1
977377a
276a39b
bf8ec74
9d41506
0ead42f
9b8e937
e5d7168
4857211
205c8c3
4962e1d
e260720
a2d73d3
b28b22f
3310eee
3178bab
901211b
6660a39
b2139c3
13e445d
66e773a
574ef2d
9c08dda
c056b01
6b264dc
67975b6
5159ad0
4d5396d
a0255b8
130dbad
e634b20
39dc618
286dbc0
e5db993
0ab3dd5
ab69a21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,8 +60,8 @@ function DynamicPPL.initialstep( | |
) | ||
# Ensure that initial sample is in unconstrained space. | ||
if !DynamicPPL.islinked(vi, spl) | ||
DynamicPPL.link!(vi, spl) | ||
model(rng, vi, spl) | ||
vi = DynamicPPL.link!!(vi, spl, model) | ||
vi = last(DynamicPPL.evaluate!!(model, vi, DynamicPPL.SamplingContext(rng, spl))) | ||
end | ||
|
||
# Define log-density function. | ||
|
@@ -79,8 +79,8 @@ function DynamicPPL.initialstep( | |
Q, _ = DynamicHMC.mcmc_next_step(steps, results.final_warmup_state.Q) | ||
|
||
# Update the variables. | ||
vi[spl] = Q.q | ||
DynamicPPL.setlogp!!(vi, Q.ℓq) | ||
vi = DynamicPPL.setindex!!(vi, Q.q, spl) | ||
vi = DynamicPPL.setlogp!!(vi, Q.ℓq) | ||
|
||
# Create first sample and state. | ||
sample = Transition(vi) | ||
|
@@ -109,8 +109,8 @@ function AbstractMCMC.step( | |
Q, _ = DynamicHMC.mcmc_next_step(steps, state.cache) | ||
|
||
# Update the variables. | ||
vi[spl] = Q.q | ||
DynamicPPL.setlogp!!(vi, Q.ℓq) | ||
vi = DynamicPPL.setindex!!(vi, Q.q, spl) | ||
vi = DynamicPPL.setlogp!!(vi, Q.ℓq) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have to redefine the log-density function since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oooo good point! Though, in that case we should probably just do away with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I guess. It seems |
||
|
||
# Create next sample and state. | ||
sample = Transition(vi) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,8 +56,8 @@ function DynamicPPL.initialstep( | |
) | ||
# Transform the samples to unconstrained space and compute the joint log probability. | ||
if !DynamicPPL.islinked(vi, spl) | ||
DynamicPPL.link!(vi, spl) | ||
model(rng, vi, spl) | ||
vi = DynamicPPL.link!!(vi, spl, model) | ||
vi = last(DynamicPPL.evaluate!!(model, vi, DynamicPPL.SamplingContext(rng, spl))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here? |
||
end | ||
|
||
# Compute initial sample and state. | ||
|
@@ -90,8 +90,8 @@ function AbstractMCMC.step( | |
newv = (1 - α) .* v .+ η .* grad .+ sqrt(2 * η * α) .* randn(rng, eltype(v), length(v)) | ||
|
||
# Save new variables and recompute log density. | ||
vi[spl] = θ | ||
model(rng, vi, spl) | ||
vi = DynamicPPL.setindex!!(vi, θ, spl) | ||
vi = last(DynamicPPL.evaluate!!(model, vi, DynamicPPL.SamplingContext(rng, spl))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
|
||
# Compute next sample and state. | ||
sample = Transition(vi) | ||
|
@@ -209,8 +209,8 @@ function DynamicPPL.initialstep( | |
) | ||
# Transform the samples to unconstrained space and compute the joint log probability. | ||
if !DynamicPPL.islinked(vi, spl) | ||
DynamicPPL.link!(vi, spl) | ||
model(rng, vi, spl) | ||
vi = DynamicPPL.link!!(vi, spl, model) | ||
vi = last(DynamicPPL.evaluate!!(model, vi, DynamicPPL.SamplingContext(rng, spl))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
end | ||
|
||
# Create first sample and state. | ||
|
@@ -238,8 +238,8 @@ function AbstractMCMC.step( | |
θ .+= (stepsize / 2) .* grad .+ sqrt(stepsize) .* randn(rng, eltype(θ), length(θ)) | ||
|
||
# Save new variables and recompute log density. | ||
vi[spl] = θ | ||
model(rng, vi, spl) | ||
vi = DynamicPPL.setindex!!(vi, θ, spl) | ||
vi = last(DynamicPPL.evaluate!!(model, vi, DynamicPPL.SamplingContext(rng, spl))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a new logdensity function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And is sampling needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As in, in that case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not have a sampler-specific evaluation context? It's mainly because we just evaluate (it seems) that I think passing around |
||
|
||
# Compute next sample and state. | ||
sample = SGLDTransition(vi, stepsize) | ||
|
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.
Can you remind me why we run the model here?
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.
And why we use the
SamplingContext
?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 think this was done to update the logp after
link!
. But nowlink!!
also includes the log-absdet-jacobian correction, so we could potentially drop it.And regarding why we use the
SamplingContext
: didn't want to make changes to something I wasn't 100% certain would have no side-effects. But yes, I also think this is no longer necessary.The question is if we should make these changes now or in another PR.
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.
Maybe safer to move these questions to a separate issue and potentially address them in a separate PR 👍