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

Add keyword arguments to blavFitIndices #43

Closed
lstmemery opened this issue Feb 13, 2022 · 9 comments
Closed

Add keyword arguments to blavFitIndices #43

lstmemery opened this issue Feb 13, 2022 · 9 comments

Comments

@lstmemery
Copy link
Contributor

Hi there,

Thank you for this incredible software. When I run blavFitIndices on one of my models I get the warning: Warning: Some Pareto k diagnostic values are too high. See help('pareto-k-diagnostic') for details.

I was able to see that there is a single k value that is 0.7, which I believe is a good candidate for moment matching. Instead of solving this issue post hoc, I would like to add a ... argument to blavFitIndices. The ... argument would pass through to loo, waic, etc. If this works for you, I could start on a PR right away.

Thanks again,

Matt

@lstmemery lstmemery changed the title Add to blavFitIndices Add keyword arguments to blavFitIndices Feb 13, 2022
@ecmerkle
Copy link
Owner

Thanks, this sounds like a good addition. I believe that blavFitIndices() is calling fitMeasures() to obtain loo and waic, so it will probably take modification of at least two functions to do this. Any initial stab you take would be welcome, and I can clean up any parts of it that are confusing.

PS I had to resist saying "This is a premium addition that can be purchased for 500 robux" :)

@lstmemery
Copy link
Contributor Author

Hey @ecmerkle,

It looks like the situation is a bit more difficult than I anticipated. Moment matching is only available when applying loo to a stanfit object. I can extract the stanfit from blavaan using fitstanfs_mcobj <- blavInspect(fitstanfs, "mcobj").

loo(fitstan_mcobj) works as expected. However, loo(fitstanfs_mcobj, moment_match = TRUE) gives the same error, regardless of the model used:

Error in object@.MISC$stan_fit_instance$unconstrain_pars(pars) : 
  Exception: Variable Lambda_y_free missing  (in 'model_stanmarg' at line 576)

For reference, this is what lines around 576 look like:

parameters {
  // free elements (possibly with inequality constraints) for coefficient matrices
  vector[len_free[1]] Lambda_y_free;
  //vector[len_free[2]] Lambda_x_free;
  vector[len_free[3]] Gamma_free;
  ...

I think loo is calling the loo_moment_match function, which in turn is calling the unconstrain_pars method attached to the stanfit. I don't know much about stan, but it looks like Lambda_y_free is already unconstrained, but I haven't come up with a way of fixing this. Do you have any ideas?

@ecmerkle
Copy link
Owner

Thanks for looking at it, and here is what I think is happening.

In the big Stan file, I post-process a number of parameters in generated quantities, and then I only save the post-processed versions (using the pars argument of stan()). I do this to save memory (to avoid saving redundant parameters). But it is also possible to save extra parameters using blavaan's mcmcextra argument. So, to check that this is the problem, I think you could do this:

  1. Compare the parameters in the Stan file to the parameters that blavaan saves by default.
  2. For things that appear in the parameters block of Stan but not in the blavaan saved parameters, save them using the mcmcextra argument. It would look roughly like
fit <- bsem(model, data, other_arguments, mcmcextra = list(monitor = c("Lambda_y_free", "more_pars"))

Then, hopefully the moment matching would work on fit. If so, I think we could still add a ..., but maybe the user has to know to also use mcmcextra. (I am not sure I want to save redundant parameters by default, though maybe I could be convinced if there were multiple functions that need everything from the parameters block.)

@lstmemery
Copy link
Contributor Author

That works! Unfortunately, moment matching is not going to save my model, but I can add changes.

It looks like loo_moment_match is only implemented for rstan models and will require all the free variables in the stan model. Because this is a pretty narrow use case, I would add an argument to the blavaan constructor called something like moment_match_k_threshold that defaults to NULL. If a value > 0.5 is input, the blavaan object will monitor all the free variables and blavFitIndices will default to loo with moment matching instead of loo. Otherwise, it will default to the typical loo.

Does that work for you?

@ecmerkle
Copy link
Owner

I'd glad it worked, even if it didn't help you! About the extra argument: I am trying to keep extra settings like this in mcmcextra. For example, there is a llnsamp argument to control the number of samples to approximate the likelihood of ordinal models; see here.

Something could be done similarly here, where moment_match_k_threshold is supplied to mcmcextra$data, then blavaan knows what to do with it. Maybe this would be easiest for me to do, because I recently did that llnsamp thing.

On the other hand, maybe the current functionality is enough since we can supply extra parameters to mcmcextra. (particularly if I add an example of how to do it, say here). Let me know what you think.

@lstmemery
Copy link
Contributor Author

I think it would make sense to add moment_match_k_threshold to mcmcdata. My reasoning is that some free variables will change depending on the modelling decisions, such as theta for ordinal models. You would also have to change bitFitIndices slightly to check for a flag. Currently, loo takes in llnsamp directly, but the model is needed when moment matching is used,

I won't stop you if you want to make the changes, but I should be able to get up PR this weekend. Up to you!

@ecmerkle
Copy link
Owner

I won't get to it by the weekend, so will build off of anything you send!

@lstmemery
Copy link
Contributor Author

Hey Ed,

I'm curious about this line. Why is the stan target not allowed here?

if(length(mcmcextra$monitor) > 0 & target != "stan"){

@ecmerkle
Copy link
Owner

I think it is because of the way that add_monitors() works inside that if statement. I think add_monitors() was written to expect JAGS results, because blavaan was written for JAGS first. The extra monitors still exist for Stan, they just are not included in the parameter table.

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

2 participants