Model changes propositions about klara.jl HOT 10 CLOSED

They all seem well-thought and useful suggestions @fredo-dedup:

• It is definitely better to remove the size argument from the model constructors, since it can be deduced from the vector size, and sure, it is good to keep it as a field.
• It is true that we repeat some parameter validation in the samplers that could be done only once in the inner constructor of the model, I haven't noticed this before.
• Having a model() function is a much more practical (and inviting) user interface, so thumbs up. Maybe we could have a named argument type in the model() function, with default value type="likelihood" (referring to the most common type MCMCLikelihoodModel), and then other boolean named arguments, such as gradient=false and hessian=false? I was thinking of a function signature along the lines model(; type::ASCIIString="likelihood", gradient::Bool="false", hessian::Bool="false"), but this naive scaffold can be thought more thoroughly...
• The scale field sounds great too.

A quick thought that I had the other day; RMHMC and MMALA need the derivatives of the Hessian, so shall we add another type MCMCLikelihoodModelWithTensor? In general, I was debating with myself wether we need all three types MCMCLikelihoodWithGradient, MCMCLikelihoodWithHessian and MCMCLikelihoodWithTensor or not really, given that they are nested and that an important part of source code is repeated there. We could have for instance MCMCLikelihoodWithTensor only, and the hessian or tensor-related fields could be empty depending on the sampler. I am not sure yet how we could improve on this, just sharing my scepticism on this part of our code.

from klara.jl.

Regarding your last point, I think there is a lot of duplicated code between those Model definitions (consistency controls, constructors). This could be a sign that we should instead create a single type with boolean fields hasGradient, hasHessian, etc.. and then have the samplers check those fields to see if the model is usable for them.
As a bonus, this would allow more readable error messages instead of the "ERROR: *no method spinTask(MCMCLikelihoodModel,HMC)" we are getting today.

What do you think ?

I could start a devel branch along those lines to check there is no stumbling blocks following this strategy leaving master intact. I would need your help for some of the models you have created though.

from klara.jl.

Yes, a single type with boolean fields seems to be the right improvement and a clearer design (as long as we don't run into any complications that we haven't thought in advance). Sure, go ahead with the devel branch and of course, I am more than happy to help with the models I created. Would you like to create the single type so that it becomes operational for the existing samplers (RWM, HMC, MALA)? Then I could take it from there to add SMMALA, MMALA, RMHMC, trying to adhere to your code, and then at a third stage you can further improve this coding addition - is this a plausible plan (or would you like me to help in another way)?

from klara.jl.

I started pursuing the idea in the devel branch. I fused basic / gradient / hessian model types into a single one.

The helper function model() is not finished yet, and I didn't finish adding tuning types to RWM().

I'll get back to this in the next days. Feel free to comment and criticize..

from klara.jl.

Thanks @fredo-dedup, no rush, I am abroad this week until 30 Aug working, so somewhat slower. I will look at your code over the weekend.

from klara.jl.

@fredo-dedup, I read your code in devel, it looks good. I attached 3 comments to it (via the commenting facility of github). Give me a shout if my thoughts there are not clear.

from klara.jl.

2 more points @fredo-dedup:

• I added the tensor field to MCMCLikelihoodModel from now because it is needed while I am looking into the process of adding the geometric MCMC samplers (RMHMC and MMALA). I made this change in devel so as to leave master in its working state.
• I realized that so far the autodiff tools are used only for computing the gradient. We certainly need to use them for computing the hessian and tensors too, whenever these are not provided by the user as functions.

from klara.jl.

@fredo-dedup I made tiny steps towards adding the tuning feature to the samplers. It takes some thinking, thus the slow (ongoing) progress. Feel free to criticize/change/improve the ongoing effort.

from klara.jl.

I fixed some bugs, and the examples work with the newly defined model-related types. I made 2 comments on my last commit in devel @fredo-dedup, let me know what you think about these remarks. I will port the empirical tuners I used for some samplers soon - feel free to make any changes, however drastic they may be.

from klara.jl.

We have made all the changes suggested in the initial list of this issue.

from klara.jl.