stan-dev / stan

Stan development repository. The master branch contains the current release. The develop branch contains the latest stable development. See the Developer Process Wiki for details.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
2.57k stars 368 forks source link

Why does transition() in hmc use ps_point and not it's actual type? #2852

Closed SteveBronder closed 2 months ago

SteveBronder commented 4 years ago

Summary:

For the code in for instance base_nuts::transition() there's a bunch of ps_point's that are being used, but the type of the derived class of ps_point can come from Hamiltonian<Model, BaseRNG>::PointType ( and that can be defined for all the hamiltonians).

i.e. there's stuff like this

// top stuff
template <class Model, template <class, class> class Hamiltonian,
          template <class> class Integrator, class BaseRNG>
class base_nuts : public base_hmc<Model, Hamiltonian, Integrator, BaseRNG> {
// yada yada

  sample transition(sample& init_sample, callbacks::logger& logger) {
    ps_point z_fwd(this->z_);  // State at forward end of trajectory
    ps_point z_bck(z_fwd);     // State at backward end of trajectory

    ps_point z_sample(z_fwd);
    ps_point z_propose(z_fwd);

Using pspoint here causes the weird line [`this->z.ps_point::operator=(z_fwd);`](https://github.com/stan-dev/stan/blob/develop/src/stan/mcmc/hmc/nuts/base_nuts.hpp#L134).

Is there a reason to use ps_point here and not the actual point type for each hamiltonian? Is there a time we would want to dynamically mix and match point types?

// stuff

Current Version:

v2.21.0

betanalpha commented 4 years ago

The base ps_point class only contains the current point in phase space, the q and the p, along with the current gradient. Derived ps_points include additional information about the local geometry as needed, specifically inverse metric components.

In order to numerically integrate the Hamiltonian trajectory we need that full local information, and hence the derived ps_point, but for most of the tree building book keeping we only need the point in phase space. In order to avoid unnecessary memory the sampler code keeps one derived ps_point instance around to evolve the trajectory and a bunch of base ps_point instances for keeping track of everything else. When creating those base instances the base operator= is used in order to copy only the basic information and not any extraneous information.

On Nov 12, 2019, at 12:22 AM, Steve Bronder notifications@github.com wrote:

Summary:

For the code in for instance base_nuts::transition() https://github.com/stan-dev/stan/blob/develop/src/stan/mcmc/hmc/nuts/base_nuts.hpp#L79 there's a bunch of ps_point's that are being used, but the type of the derived class of ps_point can come from Hamiltonian<Model, BaseRNG>::PointType ( and that can be defined for all the hamiltonians).

i.e. there's stuff like this

// top stuff template <class Model, template <class, class> class Hamiltonian, template class Integrator, class BaseRNG> class base_nuts : public base_hmc<Model, Hamiltonian, Integrator, BaseRNG> { // yada yada

sample transition(sample& init_sample, callbacks::logger& logger) { ps_point zfwd(this->z); // State at forward end of trajectory ps_point z_bck(z_fwd); // State at backward end of trajectory

ps_point z_sample(z_fwd);
ps_point z_propose(z_fwd);

Using pspoint here causes the weird line this->z.ps_point::operator=(z_fwd); https://github.com/stan-dev/stan/blob/develop/src/stan/mcmc/hmc/nuts/base_nuts.hpp#L134.

Is there a reason to use ps_point here and not the actual point type for each hamiltonian? Is there a time we would want to dynamically mix and match point types?

// stuff

Current Version:

v2.21.0

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2852?email_source=notifications&email_token=AALU3FW6TJQJCN722RNYWWDQTI4SFA5CNFSM4JL6NEP2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HYS6JIQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALU3FQCMGGD6D5LRGBW3CLQTI4SFANCNFSM4JL6NEPQ.

bob-carpenter commented 4 years ago

On Nov 14, 2019, at 9:45 AM, Michael Betancourt notifications@github.com wrote:

The base ps_point class only contains the current point in phase space, the q and the p, along with the current gradient. Derived ps_points include additional information about the local geometry as needed, specifically inverse metric components.

In order to numerically integrate the Hamiltonian trajectory we need that full local information, and hence the derived ps_point, but for most of the tree building book keeping we only need the point in phase space. In order to avoid unnecessary memory the sampler code keeps one derived ps_point instance around to evolve the trajectory and a bunch of base ps_point instances for keeping track of everything else. When creating those base instances the base operator= is used in order to copy only the basic information and not any extraneous information.

Intentionally slicing a reference is unusual enough coding style that it deserves a comment to help people perusing the code.