Feature/3181 json hmc tuning params

[mitzimorris]

Submission Checklist

• [x] Run unit tests: ./runTests.py src/test/unit
• [ ] Run cpplint: make cpplint
• [x] Declare copyright holder and open-source license: see below

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:

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

[mitzimorris]

@SteveBronder and @WardBrian - current implementation. needs unit tests for unit_e_metric. would appreciate feedback on best way to do method signature.

[mitzimorris]

will need help from C++ type ninjas.

added template type MetricWriter for services methods hmc_nuts_*_e_adapt.hpp and use structured_writer as suggested. in order to make compiler happy, unit tests must declare vectors as type structured_writer. elements added are of type json_writer - code runs but metric_writers act like no-op writers - no json output.

suggestions?

[WardBrian]

unit tests must declare vectors as type structured_writer

This doesn’t sound totally right to me. Would you mind pushing the code as you have it?

[mitzimorris]

pushed code w/ compiler error - trying

> python3 runTests.py src/test/unit/services/sample/hmc_nuts_diag_e_adapt_parallel_match_test.cpp

generates this error:

/unit/services/sample/hmc_nuts_diag_e_adapt_parallel_match_test.cpp -o test/unit/services/sample/hmc_nuts_diag_e_adapt_parallel_match_test.o
src/test/unit/services/sample/hmc_nuts_diag_e_adapt_parallel_match_test.cpp:96:21: error: no matching function for call to 'hmc_nuts_diag_e_adapt'
  int return_code = stan::services::sample::hmc_nuts_diag_e_adapt(
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/stan/services/sample/hmc_nuts_diag_e_adapt.hpp:545:5: note: candidate function template not viable: no known conversion from 'std::vector<stan::callbacks::json_writer<std::stringstream, deleter_noop>>' (aka 'vector<json_writer<basic_stringstream<char>, deleter_noop>>') to 'std::vector<callbacks::structured_writer> &' for 27th argument

[mitzimorris]

ready for re-review.

[mitzimorris]

re your question about not breaking RStan and PyStan - I think I should add back the old signature to run_adaptive_sampler.

[mitzimorris]

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.

[mitzimorris]

performance tests now failing because current CmdStan calls to adaptive sampler don't match new services API.

[WardBrian]

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

[mitzimorris]

very confused. what is the 3rd?

[WardBrian]

In develop, there are 4 overloads of hmc_nuts_diag_e_adapt.

Back in e4f62f0, there were 8 overloads of hmc_nuts_diag_e_adapt, including 4 which accepted the new argument metric_writer (I miscounted when I said three).

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

[mitzimorris]

my sad conclusion is that all of these are necessary. reverted changes.