Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moment matching with iwmm package #212

Open
n-kall opened this issue Feb 27, 2023 · 4 comments
Open

Moment matching with iwmm package #212

n-kall opened this issue Feb 27, 2023 · 4 comments

Comments

@n-kall
Copy link
Contributor

n-kall commented Feb 27, 2023

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?

@paul-buerkner
Copy link
Contributor

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.

@jgabry
Copy link
Member

jgabry commented Mar 1, 2023

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?

@n-kall
Copy link
Contributor Author

n-kall commented Mar 1, 2023

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.

@avehtari
Copy link
Collaborator

avehtari commented Mar 2, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants