Migrate to AbstractMCMC, add interface manual #1007
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 2, 2019
Merged
devmotion
reviewed
Dec 2, 2019
| end | ||
|
|
||
| # Default density constructor. | ||
| DensityModel(π::Function, data::T) where T = DensityModel{VariateForm, ValueSupport, T}(π, data) |
Member
There was a problem hiding this comment.
That seems wrong and does not match the struct definition above? Moreover, the static parameter could/should be removed (https://docs.julialang.org/en/v1/manual/style-guide/#Don't-use-unnecessary-static-parameters-1).
devmotion
reviewed
Dec 2, 2019
| end | ||
|
|
||
| # Store the new draw and its log density. | ||
| Transition(model::M, θ::T) where {M<:DensityModel, T} = Transition(θ, ℓπ(model, θ)) |
Member
There was a problem hiding this comment.
The static parameters are not needed.
devmotion
reviewed
Dec 2, 2019
| q(spl::MetropolisHastings, t1::Transition, t2::Transition) = q(spl, t1.θ, t2.θ) | ||
|
|
||
| # Calculate the density of the model given some parameterization. | ||
| ℓπ(model::DensityModel, θ::T) where T = model.ℓπ(θ) |
Member
There was a problem hiding this comment.
Same here, T could be removed.
devmotion
reviewed
Dec 2, 2019
| ts::Vector{T}; | ||
| param_names=missing, | ||
| kwargs... | ||
| ) where {ModelType<:AbstractModel, T<:AbstractTransition} |
Member
There was a problem hiding this comment.
ModelType seems wrong and should be removed, I guess, and T is not needed.
devmotion
reviewed
Dec 2, 2019
| kwargs... | ||
| ) where { | ||
| ModelType<:Sampleable, | ||
| ModelType<:AbstractModel, |
Member
There was a problem hiding this comment.
It seems all static parameters could/should be removed?
devmotion
reviewed
Dec 2, 2019
| kwargs... | ||
| ) where { | ||
| ModelType<:Sampleable, | ||
| ModelType<:AbstractModel, |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves Turing over to using AbstractMCMC as the home of the interface methods. I have also added a guide on the use of AbstractMCMC's interface methods (a modified version of the one in #920), but currently it does not include any details on how to link up a foreign algorithm to Turing. I will follow up with that document in a separate PR after #997 and #965 go through, since they seem to be providing a couple of useful tools I want to be able to point people at.
One thing to note is that I have removed the
<:Sampleabletyping on model structs in favor of a much simplerAbstractModel, since we have not been usingSampleableand it's a bit of a headache to work with.