Moment matching with iwmm package
Currently loo_moment_match only works on stanfit objects (due to the reliance on the log_prob methods from rstan), but not CmdStanFit or just a matrix of draws (see e.g. #209). I was thinking about the best way to expand support of loo_moment_match to other objects, and am opening this issue mostly for discussion at this stage.
@topipa has created a generic implementation of moment matching (iwmm) which works on a matrix or stanfit object. Recently we have also added CmdStanFit support to this package (using the new model methods in cmdstanr which are currently not yet in a release).
iwmm is not (yet) on CRAN, so now might be a good time to discuss the best way to interoperate with loo. After some discussion with @avehtari, I currently see three options (there may be others I haven't thought of):
-
update functions in loo, no change to dependencies
Code from iwmm is copied into loo and adapted for use in
loo::loo_moment_match(made specific for leave-one-out importance posteriors). iwmm would remain an independent package for generic importance sampling. -
add iwmm dependency in loo
iwmm is submitted to CRAN and
loo::loo_moment_matchis rewritten to useiwmm::moment_match. loo would then import and depend on iwmm. -
move iwmm functions into loo
All iwmm functions are moved into loo, and kept generic (i.e. not specific for leave-one-out posteriors). Keeping the functions generic has some precedence as loo is the home of
loo::psiswhich is used in cases other than leave-one-out CV (e.g. in priorsense and adjustr).
@jgabry @avehtari @topipa @paul-buerkner , do you have any thoughts on this?
From my perspective all options are good with one expection: I would prefer to not add another hard dependency of loo. That is, if iwmm was to continue its own package used by loo, it should perhaps better be under suggests instead of under imports to prevent loo from breaking if iwmm breaks.
From my perspective all options are good with one expection: I would prefer to not add another hard dependency of loo. That is, if iwmm was to continue its own package used by loo, it should perhaps better be under
suggestsinstead of underimportsto prevent loo from breaking if iwmm breaks.
I agree with @paul-buerkner. If this can work by adding a "soft" dependency then that's fine and would avoid copying code, but we should avoid adding it as a "hard" dependency. Assuming we can Suggest iwmm instead of Import it, then maybe that's the best option since it avoids copying any code between the packages, right?
Thanks for the replies.
I would prefer to not add another hard dependency of loo
Completely agree, I think it would fit under suggests fine. One potential hurdle is that iwmm currently imports loo for the pareto-k diagnostics. We'll likely need to resolve this circular dependency (even if it is a soft dependency).
Assuming we can Suggest iwmm instead of Import it, then maybe that's the best option since it avoids copying any code between the packages, right?
Yes, this should avoid code duplication. I think loo::loo_moment_match could be rewritten to use iwmm::moment_match and provide an informative error message if the iwmm namespace is not available.
We have also discussed adding Pareto-k diagnostic to posterior package (and had one person last summer to do that, but there is no PR yet), so that in cases when we just need that it would be better to depend on posterior than on loo