{"action":"created","issue":{"url":"https://api.github.com/repos/stan-dev/stan/issues/1680","labels_url":"https://api.github.com/repos/stan-dev/stan/issues/1680/labels{/name}","comments_url":"https://api.github.com/repos/stan-dev/stan/issues/1680/comments","events_url":"https://api.github.com/repos/stan-dev/stan/issues/1680/events","html_url":"https://github.com/stan-dev/stan/pull/1680","id":116151599,"number":1680,"title":"Use writer for hamiltonians","user":{"login":"syclik","id":425751,"avatar_url":"https://avatars.githubusercontent.com/u/425751?v=3","gravatar_id":"","url":"https://api.github.com/users/syclik","html_url":"https://github.com/syclik","followers_url":"https://api.github.com/users/syclik/followers","following_url":"https://api.github.com/users/syclik/following{/other_user}","gists_url":"https://api.github.com/users/syclik/gists{/gist_id}","starred_url":"https://api.github.com/users/syclik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/syclik/subscriptions","organizations_url":"https://api.github.com/users/syclik/orgs","repos_url":"https://api.github.com/users/syclik/repos","events_url":"https://api.github.com/users/syclik/events{/privacy}","received_events_url":"https://api.github.com/users/syclik/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":14,"created_at":"2015-11-10T17:01:28Z","updated_at":"2015-12-02T22:57:15Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/stan-dev/stan/pulls/1680","html_url":"https://github.com/stan-dev/stan/pull/1680","diff_url":"https://github.com/stan-dev/stan/pull/1680.diff","patch_url":"https://github.com/stan-dev/stan/pull/1680.patch"},"body":"#### Summary:\r\n\r\nThis propagates the writer to the hamiltonians.\r\n\r\n#### Intended Effect:\r\n\r\nThis is a refactor of the code.\r\nImplements more of the writer refactor in the feature/issue-1361-writer_callback branch.\r\n\r\n#### How to Verify:\r\n\r\nRun tests.\r\n\r\n#### Documentation:\r\n\r\nNone.\r\n\r\n#### Reviewer Suggestions: \r\n\r\nAnyone."},"comment":{"url":"https://api.github.com/repos/stan-dev/stan/issues/comments/161460381","html_url":"https://github.com/stan-dev/stan/pull/1680#issuecomment-161460381","issue_url":"https://api.github.com/repos/stan-dev/stan/issues/1680","id":161460381,"user":{"login":"bob-carpenter","id":3383807,"avatar_url":"https://avatars.githubusercontent.com/u/3383807?v=3","gravatar_id":"","url":"https://api.github.com/users/bob-carpenter","html_url":"https://github.com/bob-carpenter","followers_url":"https://api.github.com/users/bob-carpenter/followers","following_url":"https://api.github.com/users/bob-carpenter/following{/other_user}","gists_url":"https://api.github.com/users/bob-carpenter/gists{/gist_id}","starred_url":"https://api.github.com/users/bob-carpenter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bob-carpenter/subscriptions","organizations_url":"https://api.github.com/users/bob-carpenter/orgs","repos_url":"https://api.github.com/users/bob-carpenter/repos","events_url":"https://api.github.com/users/bob-carpenter/events{/privacy}","received_events_url":"https://api.github.com/users/bob-carpenter/received_events","type":"User","site_admin":false},"created_at":"2015-12-02T22:57:14Z","updated_at":"2015-12-02T22:57:14Z","body":"I don't think it should be in the base_mcmc class.\n\nWriters are stateful, so you have to ask yourself if\nthe state of the writer is part of a base_mcmc object.\n\nI'd think probably not and therefore you'd want to include\nthem as arguments where needed.\n\nBasically, anything that reduces statefulness of classes\nis a good thing because it's easier to reason about programs.\n\n- Bob\n\n> On Dec 2, 2015, at 4:35 PM, Daniel Lee <notifications@github.com> wrote:\n> \n> @betanalpha: I went through and added a stan::interface_callbacks::writer::base_writer argument to base_mcmc.hpp and propagated it through all places that touched it. See b4019ab. (I haven't removed the member variables.\n> \n> The reason I implemented it the way I did was because by adding base_writer to arguments of functions, it seems inconsistent when some class methods have writers and others don't. I guess this could be an indication that some of the class methods has the side effect of generating output while others don't. If that's the intention, I'm ok with it. This isn't true of all the classes. I think it's mostly true of the Hamiltonians.\n> \n> In the Hamiltonians, these functions have to have base_writer:\n> \n> \t• init\n> \t• update\n> and these don't:\n> \n> \t• T\n> \t• V\n> \t• tau\n> \t• phi\n> \t• H\n> \t• dtau_dq\n> \t• dtau_dp\n> \t• dphi_dq\n> \t• sample_p\n> I think I'd feel a little more sane if all those functions required base_writer or none or it was clear why something like sample_p doesn't need one.\n> \n> And here are the source files that changed (I excluded tests). These all had to add base_writer to an argument to make this fly. I think this was why I just started carrying around a member variable:\n> \n> \t• src/stan/mcmc/base_mcmc.hpp\n> \t• src/stan/mcmc/fixed_param_sampler.hpp\n> \t• src/stan/mcmc/hmc/base_hmc.hpp\n> \t• src/stan/mcmc/hmc/hamiltonians/base_hamiltonian.hpp\n> \t• src/stan/mcmc/hmc/integrators/base_integrator.hpp\n> \t• src/stan/mcmc/hmc/integrators/base_leapfrog.hpp\n> \t• src/stan/mcmc/hmc/nuts/adapt_diag_e_nuts.hpp\n> \t• src/stan/mcmc/hmc/nuts/adapt_unit_e_nuts.hpp\n> \t• src/stan/mcmc/hmc/nuts/base_nuts.hpp\n> \t• src/stan/mcmc/hmc/static/adapt_dense_e_static_hmc.hpp\n> \t• src/stan/mcmc/hmc/static/adapt_diag_e_static_hmc.hpp\n> \t• src/stan/mcmc/hmc/static/adapt_unit_e_static_hmc.hpp\n> \t• src/stan/mcmc/hmc/static/base_static_hmc.hpp\n> \t• src/stan/services/mcmc/sample.hpp\n> \t• src/stan/services/mcmc/warmup.hpp\n> \t• src/stan/services/sample/generate_transitions.hpp\n> \t• src/stan/services/sample/init_adapt.hpp\n> —\n> Reply to this email directly or view it on GitHub.\n> \n\n"}}