Moment matching with iwmm package
n-kall opened this issue · 4 comments
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 inloo::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 andloo::loo_moment_match
is 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 ofloo::psis
which 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
suggests
instead of underimports
to 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