stan-dev/loo

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):

  1. 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.
  2. add iwmm dependency in loo
    iwmm is submitted to CRAN and loo::loo_moment_match is rewritten to use iwmm::moment_match. loo would then import and depend on iwmm.
  3. 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::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 under imports 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