-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Allow algorithms to use draws of model fits for inits #936
Conversation
Pinging @avehtari if you have time to take this for a spin! |
After some comments from @bob-carpenter and @WardBrian in stan-dev/stan#3275 I think the default behavior should be that if any chains fail to init then all chains fail to init. This lines up with cmdstan and cmdstanpy's behavior and is usually caused by something being wrong with the model |
It seems to work. I don't see documentation on how does the init work if given a Pathfinder object with many draws. Can you clarify? |
@avehtari it uses the same logic you had in your original code. For anything that does an approximation we cut the draws down to uniques if less than 95 percent of the samples are unique.
|
@jgabry I need to minute to fix a few little things. I forgot about the performance but |
And if, e.g., 4 inits are needed, samples 4 inits from the weights using the weights and without replacement? |
No problem. I can wait to take a look (I have a bunch of other things to work on anyway this afternoon). Thanks again for working on this! |
Yep! |
The output from sampling is a bit confusing
All lines start with "Chain 1", and then have "Chain [number]" |
If I try to initialize with a wrong fit object missing a parameter
the error message is
It would be better if it would say |
|
That warning is made at a much lower level. I'll see if I can move it up
Yes. tbh I think I'm going to close this PR and open up a simpler one that just does the inits. I thought doing multi path pathfinder inits would be really messy without also enabling parallelism from within Stan, but now that I understand the codebase more I think it's actually pretty simple. To get parallelism from within cmdstan it's going to require a lot of changes and I think needs to be a separate PR
Fixed
Can you send me your |
Submission Checklist
Summary
This PR does two things
draws
object fromposterior
or fit objects from cmdstanrRemoving the need for multiple procs was done to support multiple inits for pathfinder. Previously, pathfinder was the only algorithm that used cmdstan's internal parallelism for multiple paths. While I was coding up support for pathfinder to accept a list of inits I ran into some odd problems in the lower level parts of cmdstanr that would require a bunch of if statements just for pathfinder during execution and creation/reading/writing files. imo I thought it was best to push sampling to use multiple chains as that helped keep the code a little cleaner.
Using Stan's internal parallel chains has one major downside which is that cmdstan is all or nothing for chain initialization failure. If one chain fails to initialize then the whole program fails. I have an issue (stan-dev/stan#3275) to address this, but I think personally the benefits outweigh the costs.
One other issue is that I couldn't completely remove
proc
since cmdstan does not use thegenerate_quantities
that uses Stan's internal parallelism. I have a PR (stan-dev/cmdstan#1256) to fix this so the next release of cmdstan we can use that and only need to ever spin up 1 process for many chain algorithms.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):
Simons Foundation
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: