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/3181 json hmc tuning params #3230

Merged
merged 36 commits into from Sep 21, 2023

Conversation

mitzimorris
Copy link
Member

Submission Checklist

  • Run unit tests: ./runTests.py src/test/unit
  • Run cpplint: make cpplint
  • 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:

@mitzimorris mitzimorris marked this pull request as draft September 13, 2023 12:39
@mitzimorris
Copy link
Member Author

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

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 comments to start off - most of them apply to multiple files but I only really wrote the first place I noticed them

src/stan/mcmc/hmc/base_hmc.hpp Outdated Show resolved Hide resolved
src/stan/services/sample/hmc_nuts_dense_e_adapt.hpp Outdated Show resolved Hide resolved
src/stan/services/sample/hmc_nuts_dense_e_adapt.hpp Outdated Show resolved Hide resolved
src/stan/services/sample/hmc_nuts_unit_e.hpp Outdated Show resolved Hide resolved
@mitzimorris
Copy link
Member Author

mitzimorris commented Sep 15, 2023

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

ready for re-review.

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 more questions/comments - mostly on the internals, the API/overloads of the services functions all look good at this point!

src/stan/services/sample/hmc_nuts_unit_e_adapt.hpp Outdated Show resolved Hide resolved
src/stan/services/util/mcmc_writer.hpp Outdated Show resolved Hide resolved
src/stan/services/util/run_sampler.hpp Outdated Show resolved Hide resolved
src/stan/services/util/mcmc_writer.hpp Outdated Show resolved Hide resolved
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.

Two small questions but I think this is good!

src/stan/services/sample/fixed_param.hpp Outdated Show resolved Hide resolved
@mitzimorris
Copy link
Member Author

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

@mitzimorris
Copy link
Member Author

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
Copy link
Member Author

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

@WardBrian
Copy link
Member

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
Copy link
Member Author

very confused. what is the 3rd?

@WardBrian
Copy link
Member

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
Copy link
Member Author

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

@mitzimorris mitzimorris marked this pull request as ready for review September 21, 2023 14:54
@mitzimorris mitzimorris merged commit e3089d2 into develop Sep 21, 2023
3 checks passed
@WardBrian WardBrian mentioned this pull request Jan 9, 2024
26 tasks
@mitzimorris mitzimorris deleted the feature/3181-json-hmc-tuning-params branch March 18, 2024 18:19
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.

None yet

4 participants