Removing support for AbstractMCMC.Sample piracy

[JaimeRZP]

Dear Team,

At the moment AdvancedHMC AbstractMCMC interface pirates AbstractMCMC.Samplefunction a la Turing.

Currently we are working on allowing Turing to take in AbstractMCMC.AbstractSampler as a sampler. At the moment efforts are focused on these two PR's:

• Turing: allows Turing to take external samplers.
• AdvancedHMC: provides AdvancedHMC with convinience constructors for AbstractMCMC.AbstractSampler samplers.

Once these two PR's go through users will be able to do what the current type piracy of AbstractMCMC.Sample does through Turing.

My question is, should we then remove AbstractMCMC.Sample from AdvancedHMC?

Pro's:

• It separates the roles of AdvancedHMC and Turing.
• Avoids type pyracy.

Con's

• Forces users to use Turing for something that previously could be done just within AdvancedHMC.
• Removes a functionality that users might be currently using, potentially breaking their code.

All the best, Jaime

[yebai]

Hi @JaimeRZP, I think this is a historical issue - the AHMC.sample function predates the AbstractMCMC package. We can now depreciate the AHMC.sample function and remove it in future releases. We can promote the AbstractMCMC.sample API more so users can switch to this interface.

[JaimeRZP]

Ok that's interesting. I was proposing to get rid of both but keep the instance of AbstractMCMC.sample seems reasonable to me. One thing that I am doing in #325 is to change the signature of AbstractMCMC.sample from:

function AbstractMCMC.sample(
    model::LogDensityModel,
    kernel::AbstractMCMCKernel,
    metric::AbstractMetric,
    adaptor::AbstractAdaptor,
    N::Integer;
    kwargs...,
)

to

function AbstractMCMC.sample(
    model::LogDensityModel,
    sampler::AbstractHMCSampler,
    N::Integer;
    kwargs...,
)

where AbstractHMCSampler is the umbrella type of all new constructors.

[yebai]

Do we still need this function after #325? It feels a bit redundant to me. Maybe we can depreciate it?

[JaimeRZP]

I think even though #325 is now merged. Having this function here is probably still useful if users want to use the AbstractMCMC interface without loading Turing. However, I would remove AHMC.Sample.