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

Feature/1123 laplace approx #1128

Closed
wants to merge 34 commits into from
Closed

Conversation

mitzimorris
Copy link
Member

Submisison Checklist

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

Summary:

Intended Effect:

Expose Stan service method laplace_sample to CmdStan interface, (stan-dev/stan#3148).

The laplace_sample specific arguments are:

  • draws - number of draws in sample
  • jacobian - Boolean flag, True == 1 (default), False == 0
  • mode - Either a JSON file containing estimates for all model parameters or a Stan CSV file produced by the optimize method

As part of this PR I added file src/cmdstan/command_helper.hpp for auxiliary helper methods, similar in spirit to stansummary_helper.hpp. The goal is to improve the readability and maintainability command.hpp. Further cleanup is needed, but that should be done in another PR.

How to Verify:

Unit test laplace_test

Side Effects:

N/A

Documentation:

Documentation will be added via a separate PR in the docs repo.

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): Columbia University

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

@mitzimorris
Copy link
Member Author

Filed issue #1129 to continue code cleanup effort.

Copy link
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small comments now. I think a full review should wait until after the feature freeze ends next week.

Question: should the argument to specify the Jacobians to be used for optimization be added simultaneously?

src/cmdstan/arguments/arg_laplace_mode.hpp Outdated Show resolved Hide resolved
src/cmdstan/arguments/arg_laplace_draws.hpp Outdated Show resolved Hide resolved
src/cmdstan/arguments/arg_laplace_jacobian.hpp Outdated Show resolved Hide resolved
src/test/utility.hpp Outdated Show resolved Hide resolved
@WardBrian WardBrian linked an issue Nov 15, 2022 that may be closed by this pull request
@mitzimorris
Copy link
Member Author

A few small comments now. I think a full review should wait until after the feature freeze ends next week.

agreed.

Question: should the argument to specify the Jacobians to be used for optimization be added simultaneously?

depends on how granular you like your PRs. This could be done as a sub-branch from this branch, correct?

waiting until after feature freeze would allow for more time to do more on #1129

@WardBrian
Copy link
Member

depends on how granular you like your PRs

Fair enough, my impression thus far has been that Laplace approximation is mostly useful if you were able to optimize with Jacobians. If it's useful on its own I don't think it is necessary to include in this PR or one before this

@bob-carpenter
Copy link
Contributor

Laplace approximation is mostly useful if you were able to optimize with Jacobians

Without Jacobian adjustments, optimization gives you the penalized MLE and the corresponding Laplace approximation gives you approximate standard errors.

With Jacobian adjustments, optimization gives you the max a posteriori (MAP) estimate and the corresponding Laplace approximation gives you an approximate posterior.

It's just a matter of how much we document the actual computation and how much we doc its statistical application.

@bob-carpenter
Copy link
Contributor

I should also note that the existing RStan implementation is the MLE plus standard errors version (no Jacobian adjustment). So just to recreate that feature we need the Jacobian adjustment off option for the Laplace approximation.

@mitzimorris
Copy link
Member Author

made suggested changes. will branch from this branch to work on issues #1124 and #1129

@mitzimorris
Copy link
Member Author

mitzimorris commented Nov 16, 2022

as part of thie PR, will remove duplicate "jacobian" argument classes, so that log_prob, optimize, and laplace all use same class.

not possible - method log_prob uses the more verbose name "jacobian_adjust". won't change log_prob. both optimize and laplace will use shorter arg name "jacobian", unless @bob-carpenter and/or @WardBrian would prefer name "jacobian_adjust".

@WardBrian
Copy link
Member

WardBrian commented Nov 17, 2022

not possible - method log_prob uses the more verbose name "jacobian_adjust". won't change log_prob. both optimize and laplace will use shorter arg name "jacobian", unless @bob-carpenter and/or @WardBrian would prefer name "jacobian_adjust".

I think we can change log_prob without much fear - it is not in a released version yet and shouldn't have many users. If you want to try to sneak that in before the release we could, see #1130.

If we don't want to change the existing, then I think we should go with the longer name everywhere. No reason to have two versions of the same thing

@mitzimorris
Copy link
Member Author

implementing #1124 as part of this PR - adding option for jacobian correction to optimization method.

for optimization, the default value of parameter "jacobian" is false. to implement this, under the hood, defined new arg type "arg_jacobian_false" with the correct default. this preserves backwards compatibility.

added test to check jacobians, and rewrote optimization tests, adding checks that specified algorithm is invoked.

@mitzimorris
Copy link
Member Author

ready for another round of review.

@bob-carpenter
Copy link
Contributor

@mitzimorris: It looks like there are conflicts that need to be sorted before a review.

@mitzimorris
Copy link
Member Author

Closing this, work done here included in #1134

@WardBrian WardBrian deleted the feature/1123-laplace-approx branch April 6, 2023 15:36
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

Successfully merging this pull request may close these issues.

Laplace approximation sampler command
5 participants