Add a model argument to getparams and setparams!! functions

[sunxd3]

ref: TuringLang/AdvancedHMC.jl#378 (comment)

[sunxd3]

I kept logdensity_function as the last argument so that it can be optional

[yebai]

I left some comments above. In addition, we want to consider using model::AbstractModel consistently for these APIs, since we seem to be doing them already for the sample function. See e.g., here.

[torfjelde]

I left some comments above. In addition, we want to consider using model::AbstractModel consistently for these APIs, since we seem to be doing them already for the sample function. See e.g., here.

Agree with this. This is what I've done in MCMCTempering.jl.

[torfjelde]

I second @yebai 's suggestions on the type of model passed; it should be AbstractMCMC.AbstractModel, not logdensity_function. A logdensity_function is indicated by AbstracftMCMC.LogDensityModel.

Moreover, IMO we should pass model as the first argument, not the last, and then "drop it" by default, i.e.

setparams!!(model, state, params) = setparams!!(state, params)

That way, if one needs access to the model you overload setparams(model, state, params) directly for that specific model type; otherwise, you just overload setparams!!(state, params). This will make it less likely that we'll run into method ambiguities, since the setparams!!(model, state, params) will be specific for a given model type.

[sunxd3]

Updated @yebai @torfjelde

[yebai]

I left some further comments. I am happy to discuss them.

EDIT: on second thought, I'd be happy if you want to pass model and state as two arguments instead of combining them into a tuple (model, state).

[sunxd3]

At the moment, I think pass them as two argument is more preferable for me -- just a less layer of indirections.

[sunxd3]

Thanks Hong, for AbstractMCMC, let's try to have two approvals, so I'll wait a bit for @torfjelde's opinion

[torfjelde]

Looking good! But IMO we should not have additional implementations for a logdensity_function arguments. Currently, we take the approach of passing a LogDensityModel everywhere we want to indicate that we're working with a logdensity function; we should follow this convention in setparams!! and getparams too.

[sunxd3]

No objection removing them, I added them because this what sample does. But no real reason for adding something we don't need now.

[torfjelde]

No objection removing them, I added them because this what sample does. But no real reason for adding something we don't need now.

Makes sense:) So sample is meant to be end-user facing, and so we want to make that as easy as possible. However, the methods meant for inference devs, e.g. step are all assuming a logdensity function is already wrapped in a LogDensityModel. If you're using setparams!! or getparams, this means you're already accessing step and so I'm thinking we should follow the same convention as step 👍

[sunxd3]

very much agree 👍

[torfjelde]

I'll approve because it's in a good enough state now 👍
But is how to implement this documented somewhere? And if so, should we add a note to make it clear that if you don't need access to the model you should implement setparams!!(state, params) and not setparams!!(model, state, params) ?

[sunxd3]

I assume here model follows LogDensityProblems?

[torfjelde]

No, it's a AbstractMCMC.LogDensityModel

[sunxd3]

No, it's a AbstractMCMC.LogDensityModel

This makes a lot of sense

But is how to implement this documented somewhere? And if so, should we add a note to make it clear that if you don't need access to the model you should implement setparams!!(state, params) and not setparams!!(model, state, params) ?

We definitely should, but I am not very clear on what directions to give now. I would vote for just merge this and enhance this part of the documentation later (also the interface might change again in near future). Do you think this is a bad idea?

[torfjelde]

We definitely should, but I am not very clear on what directions to give now. I would vote for just merge this and enhance this part of the documentation later (also the interface might change again in near future). Do you think this is a bad idea?

I'm happy with this given that right now my reviews are quite async (this will be better starting on Thu this coming week) since I'm in a different timezone.

Regarding the directions, I stated above

And if so, should we add a note to make it clear that if you don't need access to the model you should implement setparams!!(state, params) and not setparams!!(model, state, params) ?

That's the main thing IMO 🤷

[yebai]

Thanks @sunxd3, @torfjelde!

[torfjelde]

Good stuff @sunxd3 ! :)