Abstract mcmc rwm

[AHA-HH]

Random Walk using AbstactMCMC package

[matt-graham]

I think this method does not quite match the interface expected by AbstractMCMC.step function. Specifically step methods with two signatures need to be defined

step(rng, model, sampler, state; kwargs...)

and

step(rng, model, sampler, state; kwargs...)

with former (without state argument) called on the first step when there is no existing state and the second on all subsequent steps, and in both cases the function returning a 2-tuple with the first entry sample corresponding to the values being traced / sampled and the second entry state the current chain state.

I think something more like

Suggested change

	struct RWSampler <: AbstractMCMC.AbstractSampler
	position::Vector{Float64}
	stepsize::Float64
	end
	# Step 3 - Random Walk Metropolis Step
	function AbstractMCMC.step(
	rng::AbstractRNG,
	model::DistributionModel,
	sampler::RWSampler
	)
	proposal = sampler.position .+ sampler.stepsize .* randn(rng, length(sampler.position))
	logp_current = LogDensityProblems.logdensity(model, sampler.position)
	logp_proposal = LogDensityProblems.logdensity(model, proposal)
	log_accept_ratio = logp_proposal - logp_current
	if log(rand(rng)) < log_accept_ratio
	new_position = proposal
	else
	new_position = sampler.position
	end
	return RWSampler(new_position, sampler.stepsize), logp_current
	end
	struct RWSampler{T} <: AbstractMCMC.AbstractSampler
	stepsize::T
	end
	function AbstractMCMC.step(
	rng::AbstractRNG,
	model::DistributionModel,
	sampler::RWSampler,
	state::S
	) where {S}
	proposed_state = state .+ sampler.stepsize .* randn(rng, length(sampler.position))
	logp_current = LogDensityProblems.logdensity(model, state)
	logp_proposed = LogDensityProblems.logdensity(model, proposed_state)
	log_accept_ratio = logp_proposed - logp_current
	if log(rand(rng)) < log_accept_ratio
	new_state = proposed_state
	else
	new_state = state
	end
	return new_state, new_state
	end

would be more what we want for the case where we are updating an existing state, with we here assuming sample == state, that is we are recording the full state on each iteration (and the state is some form of vector like object).

[AHA-HH]

Hi Matt, just reviewing the comments; your suggestion makes a lot of sense, I just wanted to know if the first step function where we have no initial state should have 3 arguments as the initial state is not defined yet?

[matt-graham]

@AHA-HH - yep exactly, implementing a method for AbstractMCMC.step without a state argument that dispatches on your sampler type is intended for defining how chain is initialised. Here that could just be initialising state vector as independent samples from standard normal distribution of correct dimension (or any other arbitrary initial distribution).