Update log_prob method to take constrained_params in init format

[andrjohns]

Submisison Checklist

• Run tests: ./runCmdStanTests.py src/test
• Declare copyright holder and open-source license: see below

Summary:

Updates the recently added log_prob method so that the constrained_params argument expects its inputs to follow the same format/structure as the init argument for sampling/optimisation

Intended Effect:

Simpler usage of the log_prob method

How to Verify:

Additional tests included

Side Effects:

N/A

Documentation:

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Andrew Johnson

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

• Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
• Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

[WardBrian]

Thanks for looking into this @andrjohns. I guess the downside to this approach is losing the ability to 'batch' calls?

[andrjohns]

Thanks for looking into this @andrjohns. I guess the downside to this approach is losing the ability to 'batch' calls?

Yeah pretty much. I was thinking an option would be to have multiple files, like how the inits are handled, but I'm not sure if that would be just as messy

[bob-carpenter]

thanks for adding the doc strings for the classes. just a few minor issues left that go beyond the doc and then this is good to go

[andrjohns]

Sorry for the delay in getting to this @bob-carpenter! Ready for another look now

[bob-carpenter]

I've never understood the design of CmdStan and find the code almost impossible to read as the boundary conditions all seems strange.

My main concern is that none of this seems to be tested, but then there may be tests elsewhere.

I'd be much more comfortable if you found someone other than me who actually understands the C++ in CmdStan to review.

[andrjohns]

@WardBrian I've updated the test model to include various containers (to make that the un-constraining works correctly), and verified that the returned values are consistent with those returned by cmdstanr

[bob-carpenter]

The doc needs work here.

1. The doc says

The file can be in JSON or R Dump format, using the same structure as the 'init' argument.

What does this mean? The init argument takes a specification for a single point, whereas it looks like this function is going to allow multiple points. Something needs to specify what that format is.

2. The doc needs to be super clear about what happens to the Jacobian adjustment. Stan defines an unconstrained density by including the Jacobian for transforming from unconstrained back to constrained. This would naturally be false for providing log densities on the constrained scale, because that's the log density the user defines over constrained parameters in the model block. On the other hand, if users are expecting this to match the lp__ value that is output when the Jacobian flag is on, then that won't happen if we turn it off.

[bob-carpenter]

3. I think having the error messages talk about having wrong number of unconstrained parameters is going to be very confusing if the user has only specified parameters on the constrained scale.

Digging deeper through the source, it looks like there's an option to include or not include the Jacobian adjustment. I still think this is going to be confusing unless there is very clear doc about what the user should expect.

I see everything got changed at one point to propto = true and the gradient functional is called. This does a lot of extra work in propagating gradients that doesn't need to be done if just cast the input to var and then take the value. Then the backward pass isn't carried out.

Finally, I've never understood the structure of CmdStan, so I'd rather not be the reviewer here.

[WardBrian]

If you’d like to not be the reviewer, you will still need to remove your existing “request changes”. We can then ask @rok-cesnovar to review?

I don't feel qualified to review CmdStan code.

[bob-carpenter]

Thanks for letting me know. I just dismissed my review. I'd forgotten I looked at this a month ago and that suggestions submitted that way shouldn't be in the form of a review.

It does look like there are tests now, which is great. And the user-facing doc I think is important to clarify should go in the CmdStan docs, not here in the code.

[rok-cesnovar]

Looks good to me. Some user facing doc would be great, but that is outside of the scope of this repo.