-
-
Notifications
You must be signed in to change notification settings - Fork 374
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/3181 json hmc tuning params #3230
Conversation
@SteveBronder and @WardBrian - current implementation. needs unit tests for unit_e_metric. would appreciate feedback on best way to do method signature. |
…com/stan-dev/stan into feature/3181-json-hmc-tuning-params
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.
A few comments to start off - most of them apply to multiple files but I only really wrote the first place I noticed them
will need help from C++ type ninjas. added template type MetricWriter for services methods suggestions? |
This doesn’t sound totally right to me. Would you mind pushing the code as you have it? |
pushed code w/ compiler error - trying
generates this error:
|
…com/stan-dev/stan into feature/3181-json-hmc-tuning-params
…com/stan-dev/stan into feature/3181-json-hmc-tuning-params
ready for re-review. |
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.
A few more questions/comments - mostly on the internals, the API/overloads of the services functions all look good at this point!
src/test/unit/services/sample/hmc_nuts_dense_e_adapt_parallel_match_test.cpp
Outdated
Show resolved
Hide resolved
src/test/unit/services/sample/hmc_nuts_dense_e_adapt_parallel_match_test.cpp
Outdated
Show resolved
Hide resolved
…com/stan-dev/stan into feature/3181-json-hmc-tuning-params
…com/stan-dev/stan into feature/3181-json-hmc-tuning-params
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.
Two small questions but I think this is good!
re your question about not breaking RStan and PyStan - I think I should add back the old signature to run_adaptive_sampler. |
…com/stan-dev/stan into feature/3181-json-hmc-tuning-params
for multi-chain adaptive NUTS-HMC, there are now only 2 signatures: with and without pre-specified metric, both of which now also have add'l arg for metric_writer. updated the unit tests accordingly. if CmdStan is the only interface that calls these methods, this shouldn't mess anything up. |
…com/stan-dev/stan into feature/3181-json-hmc-tuning-params
performance tests now failing because current CmdStan calls to adaptive sampler don't match new services API. |
Yes, I think I was unclear in what I was asking before. This PR definitely needs to introduce (at least) two new overloads to each of the adaptive samplers - but I believe it was introducing three, so I was wondering if the third was a strict requirement |
very confused. what is the 3rd? |
In develop, there are 4 overloads of Back in e4f62f0, there were 8 overloads of However, of those 4 original overloads, I don't believe all of them are really in use besides for backwards compatibility, since several just call each other with extra arguments. If that is the case, we don't need to (and I would argue, should not) add functionality to the overloads which are already exposing some subset of the full feature set. I haven't tried it myself, which is why I was asking if they were all necessary |
This reverts commit af33766.
…com/stan-dev/stan into feature/3181-json-hmc-tuning-params
my sad conclusion is that all of these are necessary. reverted changes. |
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
Add hooks to adaptive sampler methods to write adapted metric to JSON file.
Intended Effect
Make it easier to get adapted metric.
How to Verify
Unit tests
Side Effects
N/A
Documentation
N/A
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: