rcppsmc / rcppsmc

Rcpp bindings for Sequential Monte Carlo
25 stars 13 forks source link

Using inheritance for the moveset #45

Closed LeahPrice closed 3 years ago

LeahPrice commented 6 years ago

Currently the package uses inheritance for parameter updates and pointers to functions for the moveset. This pull request is about switching to inheritance for the moveset while maintaining backwards compatibility.

The biggest change is in the moveset.h file, followed by the sampler.h file. All examples had to be changed (in a similar way). I'm not sure what's up with the diff file for LinReg_LA.cpp (the actual changes are similar to other examples).

eddelbuettel commented 6 years ago

I'm not sure what's up with the diff file for LinReg_LA.cpp (the actual changes are similar to other examples).

"Whitespace" -- often line endings, sometimes affected by git settings. (You can do something about auto-conversion in your git settings.) For the display, I just tried

Changes look fine from 20'000 feet, not sure I have the stomach to look at them change by change :)

LeahPrice commented 6 years ago

Ah, great! Thanks for that tip.

adamjohansen commented 6 years ago

Great stuff.

At the risk of making a slightly belated suggestion, might it make sense to move the iteration over particles into the moveset class with additional virtual functions for each move type? This would then provide a clean mechanism by which users could choose to do something at a population level instead of specifying only a particle level move while being completely transparent to people who don't want to do anything more complicated than particle-level moves?

After our earlier conversation it might seem like that's not going to be useful without doing something to allow for more efficient linear algebra but I have in mind things like sequential MCMC / MCMC-particle filters and other situations in which the proposals aren't conditionally iid as a potential use case.

I'd be happy to discuss tomorrow; or we could leave things as they are -- it would be a nice extra feature but certainly isn't essential.

eddelbuettel commented 6 years ago

Hm, not sure. I can see this being useful either way. Maybe least resistance and not do it now? Or do it now while the energy level is high?

LeahPrice commented 6 years ago

Thanks, that's true. I didn't think about the case of proposals that aren't conditionally independent. DoInit, DoMove and DoMCMC are the functions that loop over iterations and fortunately they're already in moveset. DoInit and DoMove are simple loops over iterations. DoMCMC also iterates over the number of MCMC repeats (if greater than 0) and keeps track of the total acceptances. I think all that's required is too add virtual in front of these functions. It won't take too long to check it out so I think it's worth it.

LeahPrice commented 6 years ago

It tested this and it works. Do you think we should have one or more examples in the library that do the population level moves? I have files for pflineart and LinReg that do this but I haven't included them in this commit because it seems more natural to keep them as particle level moves.