Minor issues with new log_prob, expose_functions and other new methods

[jgabry]

@andrjohns A few things about the awesome new methods you added (sorry I didn't have a chance to comment on these during the review of the PRs, I was overloaded with other work at the time and am only having a chance to really play around with these now):

Errors in examples

The examples in the doc for log_prob, grad_log_prob, constrain_pars, and unconstrain_pars all result in errors:

fit_mcmc <- cmdstanr_example("logistic", method = "sample")
fit_mcmc$log_prob(upars = c(0.5, 1.2, 1.1, 2.2, 1.1))

Error: The method has not been compiled, please call `init_model_methods()` first

So I think we need to insert a line calling init_model_methods(), right?

For the grad_log_prob example I also get:

Error: Model has 4 unconstrained parameter(s), but 5 were provided!

Convenience of log_prob interface

I also think we need to provide some examples of how to use these methods together in a more realistic setting because users won't be able to just type in numbers for upars, they'll need to create structured input for unconstrain_pars and then pass the result of unconstrain_pars to log_prob, etc. Or can we provide some easier interface that combines these steps automatically or some other helper functions to make it smoother? People have always complained about how hard the methods in RStan are to use, so we don't necessarily need to copy those exactly here, we could wrap them in something more convenient if that's possible.

Documentation for expose_functions

I mentioned this in #590 (comment) but I think we're missing documentation on how users can access the functions from the functions block that get compiled when compile_standalone=TRUE (i.e., via mod$functions). I also don't think expose_functions is documented anywhere.

---

Thanks again for implementing these and sorry for the slow feedback!

[jgabry]

People have always complained about how hard the methods in RStan are to use, so we don't necessarily need to copy those exactly here, we could wrap them in something more convenient if that's possible.

In fact, one of the purposes of CmdStanR is to avoid making a lot of mistakes that we made with RStan ;)

[rok-cesnovar]

am only having a chance to really play around with these now

This is why I have refrained from making a minor release for all this new stuff.

[jgabry]

am only having a chance to really play around with these now

This is why I have refrained from making a minor release for all this new stuff.

Thanks. I finally have a bit more time now!

[andrjohns]

Oh excellent catch, thanks Jonah! I'll update the examples and draft some vignettes for the model methods and expose-functions over the next few days.

In terms of simplifying the approach compared to rstan, I definitely agree that the approach used for passing parameters to log_prob/grad_log_prob/hessian is a bit clumsy.
Like you said, the approach currently matches rstan, where the user has to call unconstrain_pars first and then pass the result:

fit_mcmc <- cmdstanr_example("logistic", method = "sample")
fit_mcmc$init_model_methods()
upars <- fit_mcmc$unconstrain_pars(pars=list("alpha"=0.1, "beta"=c(-0.8, -0.2, 0.3))) 
fit_mcmc$log_prob(upars = upars)

I think this would be a bit more intuitive if the log_prob input was changed to pars, and then internally we detect whether a single numeric vector of values (i.e., already-unconstrained pars), or a named list of values was passed, and then appropriately unconstrain or not before calling log_prob:

cpars <- list("alpha"=0.1, "beta"=c(-0.8, -0.2, 0.3))
upars <- fit_mcmc$unconstrain_pars(pars = cpars) 

# The following should return identical values
fit_mcmc$log_prob(pars = cpars)
fit_mcmc$log_prob(pars = upars)

How does that sound?

[jgabry]

Oh excellent catch, thanks Jonah! I'll update the examples and draft some vignettes for the model methods and expose-functions over the next few days.

Awesome, thanks!

How does that sound?

I like the idea of log_prob accepting constrained or unconstrained, although I suppose that could be confusing in its own way. @rok-cesnovar What do you think? For now I don't even mind keeping the two steps of unconstraining and then calling log_prob, because I actually think the most annoying step for users won't be calling two functions instead of one, but rather creating the structured list of parameter values (for non-trivial models).

So I like that you added a way to just get the "skeleton", but what if the skeleton thing were its own method instead of just an argument to constrain_pars? Right now the user needs to call constrain_pars just to get the skeleton for unconstrain_pairs, which is awkward because in the workflow it reads as if the parameters and being constrained and then unconstrained:

fit_mcmc <- cmdstanr_example("logistic", method = "sample")
fit_mcmc$init_model_methods()
skel <- fit_mcmc$constrain_pars(skeleton_only = TRUE)
skel$alpha <- 1
skel$beta <- rep(1, 3)
upars <- fit_mcmc$unconstrain_pars(skel)
fit_mcmc$log_prob(upars)

Maybe instead calling fit_mcmc$constrain_pars(skeleton_only = TRUE) the user can do fit_mcmc$variable_skeleton() or something like that? (Also, we've been using variables instead of pars throughout CmdStanR so maybe we should also change uses of pars in these methods for consistency).

Anyway, just an idea, I'm definitely open to other options.

[n-kall]

I've been using these new methods a fair amount lately for implementing cmdstanr support in the iwmm package. In addition to extending a big thanks to @andrjohns for these methods, I thought I'd add one thought on the interface:

A common task in the iwmm package that could be easier is to unconstrain / constrain all the draws from a fitted model. Currently (I think) this requires a fair amount of wrapping code, see here.

I imagine there are other cases in which (un)constraining all draws is something that is useful. So I was wondering if this is something that could be implemented on the cmdstanr side, considering that the interface does not need to match rstan's unconstrain_pars /
constrain_pars methods directly?

[mitzimorris]

cf stan-dev/cmdstan#1133

[andrjohns]

Good ideas all! I completely agree @jgabry and @n-kall, I'll make those updates

@mitzimorris the cmdstanr methods don't use the cmdstan log_prob function, so that's not an issue here

[singmann]

Sorry for coming to this somewhat late, but in the bridgesampling package we also need to unconstrain the full set of parameters (for which we so far use rstan::unconstrain_pars). So doing this easily for all samples would help us a lot in adding cmdstanr support.

Just FYI, we then pass new samples of unconstrained parameters to rstan::log_prob. So would appreciate if that function also supports samples in unconstrained space.

[jgabry]

Closing since I think these things were implemented in various PRs. Feel free to reopen if we're still missing anything.