-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Finer control of solve
and sample
args via turing_inference
args
#328
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great @saumil-sh! I don't really understand why the stan_inference cannot also be made to move to this though, can you say more about that?
Also if we are going to do a breaking release might be worth to consider if we want the SciML interface implemented here |
Boy, I am glad to hear that this doesn't make sense only in my head! That is correct; there is no reason why What is your idea of a SciML interface here? like one This discussion is fantastic, actually; a common interface could facilitate using |
Yeah so we should update that as well, if you don't mind putting it in this PR!
Yes, but more inline with the interface of other packages - we'd have structs |
Okay, I don't have stan installed at the moment. I will try to modify EDIT 1: Installing CmdStan 🚀 |
Update: Which EDIT 1: The CI/CD tests are run using |
Feel free to bump the CmdStand being used on CI in that case. Also now the julia package that serves as the interface to cmdstan is StanSample.jl not CmdStan.jl so just take a look at that as well DifEqBayesStan was moved into a separate repository because Rob wanted to do some testing and benchmarking on his own. I had mostly kept up with the updates here so the syntax shouldn't need to be updated much? |
I copied over some lines from I have done everything I can here on this PR. I wonder why documentation and format checks are failing. I ran format checks from my VS code, which uses JuliaFormatter.jl 🤷 |
This fell out of my attention, thanks for updating! |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
The
turing_inference
function interacts withsolve
andsample
; however, theturing_inference
keyword arguments are supplied only to the differential equation solver throughsolve
. There is no way to fine-tune the hyper-parameters of the sampling algorithm. The only way is to modify the source. This may present beginners with a steeper learning curve. With this pull request, I change theturing_inference
arguments to explicitly takesolve_kwargs
,sample_args
, andsample_kwargs
. Defaults are set for anyone wanting off-the-shelf inference, and power users can modify everything to their liking. This change comes at a cost: divergence from thestan_inference
argument structure and a minimal breaking change forturing_inference
. It could be an excellent time to add this now with MTK v9 when everyone updates their code.