search

tpapp / DynamicHMC.jl

Implementation of robust dynamic Hamiltonian Monte Carlo methods (NUTS) in Julia.
Other
243 stars 21 forks source link

Fix rand_phasepoint for non-Euclidean KE #55

Open sethaxen opened 5 years ago

sethaxen commented 5 years ago

rand_phasepoint should pass the point q to rand. i.e. https://github.com/tpapp/DynamicHMC.jl/blob/65f8a30ed0cc74c26206dcbbdc6dee91629dfddf/src/hamiltonian.jl#L157 should be

rand_phasepoint(rng::AbstractRNG, H, q) = phasepoint_in(H, q, rand(rng, H.κ, q))

This is consistent with the existing interface for rand used in

https://github.com/tpapp/DynamicHMC.jl/blob/65f8a30ed0cc74c26206dcbbdc6dee91629dfddf/src/hamiltonian.jl#L97

and enables the user to use rand_phasepoint with user-defined non-EuclideanKEs.

tpapp commented 5 years ago

Thanks for reporting this. This code was (is being) reorganized in #44, but I will think about how to expose this.

Do you use or want to use non-Euclidean KE's with DynamicHMC?

sethaxen commented 5 years ago

Do you use or want to use non-Euclidean KE's with DynamicHMC?

Yes, I'm writing a package for HMC on manifolds that uses most of DynamicHMC's building blocks as mentioned in https://github.com/tpapp/DynamicHMC.jl/issues/43. Much of the interface here is sufficiently general that no changes are necessary, which is fantastic. I'll take a look at #44.

tpapp commented 5 years ago

I am very happy to support this in general, and take PRs against https://github.com/tpapp/DynamicHMC.jl/tree/tp/major-api-rewrite-2.0 (the branch where #44 is conducted).

The code should be much cleaner, feel free to ask questions about it here.

tpapp commented 5 years ago

Just a heads-up: everything in that PR is now merged, with subsequent improvements, and now I am preparing for a 2.0 release (#69).

This part of the API is deliberately not exported so we can always change/extend it without breaking anything if that's what you need. I am happy to take PRs about this.

sethaxen commented 5 years ago

Thanks! Sorry I hadn't replied yet. Shortly after my last comment, I realized that the simple expression of the generalized NUTS criterion on Riemannian manifolds in Betancourt's paper presumes one has a global coordinate system on the manifold. For general manifolds, even the sphere, this is not the case, and one will somehow need to parallel transport rho. This would at least mean that leapfrog would need to access rho, but it will likely be more involved. I haven't worked out how to do that yet in a way that's consistent with the theory, so this project is on hold for the moment.

Congrats on the great work! Looking forward to 2.0!