mandymejia / BayesfMRI

BayesfMRI R package
GNU General Public License v3.0
24 stars 7 forks source link

Update BayesGLM.R #18

Closed smeisler closed 2 years ago

smeisler commented 2 years ago

Change the INLFA f model parameter spde to be a string, so it can be recognized as an INLA model input. INLA f documentation. Issue described in https://github.com/mandymejia/BayesfMRI/issues/17.

danieladamspencer commented 2 years ago

Hi @smeisler ,

I tried your fix on my Ubuntu machine, and it changed the error from object 'spde' not found to Argument 'n' in f() is required for model: spde. I'll accept your pull request, but I think more work needs to be done. Our collaborator @damondpham will catch up on this issue and help you from here.

Thanks again! Dan

mandymejia commented 2 years ago

Hi Steven and Dan,

I haven't had a chance to look at the code yet, but I believe spde may be an object we construct and pass into the function.

Mandy

On Sat, Jan 22, 2022, 8:56 AM Dan @.***> wrote:

Hi @smeisler https://github.com/smeisler ,

I tried your fix on my Ubuntu machine, and it changed the error from object 'spde' not found to Argument 'n' in f() is required for model: spde. I'll accept your pull request, but I think more work needs to be done. Our collaborator @damondpham https://github.com/damondpham will catch up on this issue and help you from here.

Thanks again! Dan

— Reply to this email directly, view it on GitHub https://github.com/mandymejia/BayesfMRI/pull/18#issuecomment-1019274106, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHVKUQXF4XTFENRB22TF53UXKZSFANCNFSM5MQXJCEQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

damondpham commented 2 years ago

Yes, spde is an object we construct in the BayesGLM function:

# Line 804 on master, and 1048 on 1.8EM:
spde <- inla.spde2.matern(mesh)

But this change patches the formula arg to estimate_model, not the spde arg:

formula_vec <- paste0('f(',beta_names, ', model = "spde", replicate = ', repl_names, hyper_vec, ')')
formula_vec <- c('y ~ -1', formula_vec)
formula_str <- paste(formula_vec, collapse=' + ')
formula <- as.formula(formula_str, env = globalenv())

We still pass the spde object to the spde argument of estimate_model:

      INLA_result <- estimate_model(
        formula=formula, data=model_data, A=model_data$X, spde, prec_initial=1,
        num.threads=num.threads, verbose=verbose, contrasts = contrasts,
        keep = keep, twostage = twostage
      )

So this pull request looks good to me!

damondpham commented 2 years ago

Actually, sorry, I misunderstood. I think this change should be reversed, based on this thread?

https://groups.google.com/g/r-inla-discussion-group/c/-L6rYho4F3k

mandymejia commented 2 years ago

Yes, I believe that the object spde passed into estimate_model is expected in the formula passed to the inla function within estimate_model.

So unless there's been a major change in INLA, I believe this change should be reversed.

On Sat, Jan 22, 2022, 10:12 PM Damon Pham @.***> wrote:

Actually, sorry, I misunderstood. I think this change should be reversed, based on this thread?

https://groups.google.com/g/r-inla-discussion-group/c/-L6rYho4F3k

— Reply to this email directly, view it on GitHub https://github.com/mandymejia/BayesfMRI/pull/18#issuecomment-1019403228, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHVKUQF3M2MJL7P35EYIXDUXNWYLANCNFSM5MQXJCEQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>