-
Notifications
You must be signed in to change notification settings - Fork 5
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
General improvements and fixes #133
Changes from 27 commits
4d48a35
2df4a2b
83c5c49
9e9153c
26f12f1
aa88ae4
2523a9d
7462ebf
3fb13e9
90e60f2
e30718d
9e8607a
3bb79df
90722c9
50ad1ec
0c204ac
e2bbc90
e7466cc
0a59517
f7a7f31
696d8d1
bbb2fc2
8ac7374
f7c46e7
dcb2a0d
287b501
6239710
8c1b8ff
f70690f
afa2900
c0d9e61
4563d33
86fbf0d
229059b
c4583f9
a1efd11
c221572
632cca9
f30acfa
0a0b131
910dc6d
a4437f9
b9c6a90
79cbbf0
4336516
915b610
4898193
13cb3b2
cb6937e
db4b842
fce26aa
8a6b47e
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 |
---|---|---|
@@ -1,20 +1,16 @@ | ||
|
||
struct PolynomialStep | ||
η :: Real | ||
c :: Real | ||
@concrete struct PolynomialStep | ||
η | ||
c | ||
end | ||
function get(step::PolynomialStep, k::Real) | ||
step.c * (k + 1.) ^ (-step.η) | ||
end | ||
|
||
|
||
struct AdaptiveState | ||
swap_target_ar :: Real | ||
scale :: Base.RefValue{<:Real} | ||
step :: PolynomialStep | ||
end | ||
function AdaptiveState(swap_target::Real, scale::Real, step::PolynomialStep) | ||
AdaptiveState(swap_target, Ref(log(scale)), step) | ||
struct AdaptiveState{T1<:Real,T2<:Real,P<:PolynomialStep} | ||
swap_target_ar :: T1 | ||
logscale :: T2 | ||
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.
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. The Julia community is quite fond of squash-case, hence why I did it like this. 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. Wasn't aware of squash case in variable names/struct fields, just in function names, but makes sense. |
||
step :: P | ||
end | ||
|
||
|
||
|
@@ -26,15 +22,15 @@ function init_adaptation( | |
) | ||
Nt = length(Δ) | ||
step = PolynomialStep(γ, Nt - 1) | ||
Ρ = [AdaptiveState(swap_target, scale, step) for _ in 1:(Nt - 1)] | ||
Ρ = [AdaptiveState(swap_target, log(scale), step) for _ in 1:(Nt - 1)] | ||
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. Perhaps the function should take 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. Yep, this is essentially what's holding the PR back atm. This was left-over from before, not something I introduced; I've just tried to make it consistent (pretty sure the previous impl was buggy). But I need to find the source of the adaptation method so I can figure out exactly what the correct impl is. |
||
return Ρ | ||
end | ||
|
||
|
||
function rhos_to_ladder(Ρ, Δ) | ||
β′ = Δ[1] | ||
for i in 1:length(Ρ) | ||
β′ += exp(Ρ[i].scale[]) | ||
β′ += exp(Ρ[i].logscale) | ||
Δ[i + 1] = Δ[1] / β′ | ||
end | ||
return Δ | ||
|
@@ -48,7 +44,9 @@ function adapt_rho(ρ::AdaptiveState, swap_ar, n) | |
end | ||
|
||
|
||
function adapt_ladder(Ρ, Δ, k, swap_ar, n) | ||
Ρ[k].scale[] += adapt_rho(Ρ[k], swap_ar, n) | ||
return Ρ, rhos_to_ladder(Ρ, Δ) | ||
end | ||
function adapt_ladder(P, Δ, k, swap_ar, n) | ||
P[k] = let Pk = P[k] | ||
@set Pk.logscale += adapt_rho(Pk, swap_ar, n) | ||
end | ||
return P, rhos_to_ladder(P, Δ) | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,46 +1,37 @@ | ||
""" | ||
get_scaling_val(Nt, swap_strategy) | ||
|
||
Calculates the correct scaling factor for polynomial step size between temperatures | ||
Calculates the correct scaling factor for polynomial step size between temperatures. | ||
""" | ||
function get_scaling_val(Nt, swap_strategy) | ||
# Why these? | ||
if swap_strategy == :standard | ||
scaling_val = Nt - 1 | ||
elseif swap_strategy == :nonrev | ||
scaling_val = 2 | ||
else | ||
scaling_val = 1 | ||
end | ||
return scaling_val | ||
end | ||
|
||
get_scaling_val(Nt, ::StandardSwap) = Nt - 1 | ||
get_scaling_val(Nt, ::NonReversibleSwap) = 2 | ||
get_scaling_val(Nt, ::RandomPermutationSwap) = 1 | ||
|
||
""" | ||
generate_Δ(Nt, swap_strategy) | ||
generate_inverse_temperatures(Nt, swap_strategy) | ||
|
||
Returns a temperature ladder `Δ` containing `Nt` temperatures, | ||
generated in accordance with the chosen `swap_strategy` | ||
generated in accordance with the chosen `swap_strategy`. | ||
""" | ||
function generate_Δ(Nt, swap_strategy) | ||
function generate_inverse_temperatures(Nt, swap_strategy) | ||
scaling_val = get_scaling_val(Nt, swap_strategy) | ||
Δ = zeros(Real, Nt) | ||
Δ = zeros(Nt) | ||
Δ[1] = 1.0 | ||
β′ = Δ[1] | ||
for i ∈ 1:(Nt - 1) | ||
β′ += exp(scaling_val) | ||
β′ += scaling_val | ||
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. Should this be |
||
Δ[i + 1] = Δ[1] / β′ | ||
end | ||
return Δ | ||
end | ||
|
||
|
||
""" | ||
check_Δ(Δ) | ||
check_inverse_temperatures(Δ) | ||
|
||
Checks and returns a sorted `Δ` containing `{β₀, ..., βₙ}` conforming such that `1 = β₀ > β₁ > ... > βₙ ≥ 0` | ||
""" | ||
function check_Δ(Δ) | ||
function check_inverse_temperatures(Δ) | ||
if !all(zero.(Δ) .≤ Δ .≤ one.(Δ)) | ||
error("Temperature schedule provided has values outside of the acceptable range, ensure all values are in [0, 1].") | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
""" | ||
make_tempered_model(sampler, model, args...) | ||
|
||
# struct TemperedModel <: AbstractPPL.AbstractProbabilisticProgram | ||
# model :: DynamicPPL.Model | ||
# β :: AbstractFloat | ||
# end | ||
Return an instance representing a model. | ||
|
||
The return-type depends on its usage in [`compute_tempered_logdensities`](@ref). | ||
""" | ||
function make_tempered_model end | ||
|
This file was deleted.
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.
Any reason not to
@concrete
this? I think I'd prefer it for consistency.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 didn't do it here because in this cause I think the type-restrictions are reasonable, but I'm not super-invested either way 🤷