stan-dev / shinystan

shinystan R package and ShinyStan GUI
https://mc-stan.org/shinystan
GNU General Public License v3.0
197 stars 51 forks source link

Letting user pass monitor object with array #175

Closed Cole-Monnahan-NOAA closed 4 years ago

Cole-Monnahan-NOAA commented 4 years ago

I use shinystan for arrays all of the time, and the models tend to be big enough that calculating the monitor is not trivial. I also typically do it outside of shinystan so that I can extract and look at things manually. But when I then do as.shinystan() it calls on the monitor function and recalculates it:

https://github.com/stan-dev/shinystan/blob/master/R/shinystan-objects.R#L206

I'm wondering if you could change this to an optional argument, and pass through here to avoid having to calculate it twice? Also, how exactly is this different than `rstan::monitor'?

I could fork and test on my end and do a pull request if this is something you're willing to allow.

VeenDuco commented 4 years ago

I'll take a look at how we could adapt this for you. I need to check where we require this part of the shinystan object to be filled in. Do you have an example that I could use? That way I can try to figure out multiple ways to speed thing up for you.

Cole-Monnahan-NOAA commented 4 years ago

Hi @VeenDuco I forked the repo and tried it out. Happy to do a pull request if you are OK with that: https://github.com/Cole-Monnahan-NOAA/shinystan/tree/develop

Apologies for the line ending changes (advice on removing those?), but the core change is here

The default is backwards compatible, but now the user can pass an Rstan monitor object. A wrong object throws an informative error. I also updated the argument documentation.

I'm assuming that the differences in the summary between ShinyStan and Rstan are negligible?

Another benefit is also that if you launch shinystan then close it and then launch it again it doesn't have to recalculate the summary.

Test code:

## Create a fake object
X <- array(rnorm(9*1e4), dim=c(1e4, 3,3),
           dimnames=list(NULL,NULL,c('x1', 'x2', 'lp__')))
warmup <- 500
mon <- rstan::monitor(X, warmup=warmup, print=FALSE)

sso1 <- shinystan::as.shinystan(X, warmup=warmup,
             algorithm='RWM', model_name='dummy')
sso2 <- shinystan::as.shinystan(X, warmup=warmup,
             algorithm='RWM', model_name='dummy', summary=mon)
sso3 <- shinystan::as.shinystan(X, warmup=warmup,
             algorithm='RWM', model_name='dummy', summary='bad')

sso1@summary
sso2@summary
VeenDuco commented 4 years ago

Hello @Cole-Monnahan-NOAA,

I see you forked the current master branch (version 2.5.0). We are working on a version 3.0, see here. It looks like your solution could be implemented there in a straightforward way too!

I think you should be able to create a pull request for that specific branch. That would be preferable so you get the credits for the contribution :). If not, let me know and I will add the solution in manually.

Great to have you contributing, thank you so much!

Best, Duco