Implementation of Robust Adaptive Metropolis

[torfjelde]

This PR introduces a simple implementation of Robust Adaptive Metropolis (RAM) that I have lying around.

This makes use of the more recent AbstractMCMC.step_warmup, which is meant to be used for exactly adaptation scenarios like this. The number of adaptation steps is therefore specified by num_warmup passed to sample.

Note that there have been a few previous attempts at adding adaptive MH samplers to AdvancdeMH.jl, but they've all "died out" as the complexity of adding such functionality to the existing "interface" is somewhat complicated. I would suggest we merge this as is (as I don't have enough time to push this to 100%), and then we later PRs can deal with either extending this to be more general and / or integrate it with the existing "interface".

Related: #91 #39 #57

[torfjelde]

Maybe you could have a look at this @devmotion ?:)

[mhauru]

Code looks mostly good, I just had a comments on documentation and testing. Can't comment on the maths in adapt since I haven't read the paper/understood the theory.

[torfjelde]

Added proper testing @mhauru ; thanks for the review:)

[devmotion]

Maybe ensure consistent types here as well?

Suggested change

	x = initial_params === nothing ? rand(rng, d) : initial_params
	x = initial_params === nothing ? rand(rng, eltype(sampler.γ), d) : initial_params

By the way, rand(rng, d) doesn't seem a good choice in general? The algorithm requires that you start with a point in the support of the target distribution and it's not clear if the target density is zero for this point. I wonder if it requires something like https://github.com/TuringLang/EllipticalSliceSampling.jl/blob/3296ae3566d329207875216837e65eeec3b809b2/src/interface.jl#L20-L29 in EllipticalSliceSampling.

[torfjelde]

But this is using a RWMH as the main kernel, so IMO we're already assuming unconstrained support for this to be valid

[torfjelde]

And more generally, happy to deal with better initialisation, buuuuut prefer to do this in a separate PR as I'm imagining this will require some discussion

[devmotion]

so IMO we're already assuming unconstrained support

But why prefer rand over randn in that case?

But I agree, probably this question (rand/randn/dedicated API) should be addressed separately.

[torfjelde]

But why prefer rand over randn in that case?

Just did it quickly because I have some vague memory that it's generally preferred to do initialisation in a box near 0 for most of the linking transformations (believe this is the moticvation behind SampleFromUniform in DPPL, though I think it technically initialses from a cube centered on 0?).

But it's w/e to me here; we need a better way in general for this, so I'll just change it to randn 👍

[torfjelde]

Did most of what you suggested @devmotion; see if you're okay with it now:)

I don't have much time these days, so I would strongly suggest that either we merge it as is or someone else takes over this PR 👍

[mhauru]

"Request changes" because the quadruple backtick seems worth fixing, happy to merge once that is done. I left a couple of other comments too, but they are optional to attend to.

[mhauru]

That looks like one backtick too many. PS. How do I make a GitHub suggestion for this?

[penelopeysm]

````suggestion
```
````

if you have N backticks surround them with N+1 😄

[mhauru]

On second thought, I just pushed a fix for this myself.

[mhauru]

Thanks @penelopeysm. :) I should have guessed that the universe would have been organised in such a way that the fix to quadruple backticks would be more quadruple backticks.

[mhauru]

Has JuliaFormatter been run? Just wondering because I thought the style guide was to not have whitespace around kwarg defaults. Not important though, can be left as is.

[mhauru]

Fixed the backticks myself, the rest are optional.

[torfjelde]

Thanks @mhauru and @devmotion !