-
Notifications
You must be signed in to change notification settings - Fork 46
Make ∂H∂r consistent with ∂H∂θ #458
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
base: main
Are you sure you want to change the base?
Conversation
AdvancedHMC.jl documentation for PR #458 is available at: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #458 +/- ##
==========================================
- Coverage 75.44% 75.42% -0.03%
==========================================
Files 21 21
Lines 1230 1233 +3
==========================================
+ Hits 928 930 +2
- Misses 302 303 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, @ErikQQY. Let's mark this as a breaking release with a clear item in HISTORY.md, since some downstream pages might depend on neg_energy
.
HISTORY is updated and a minor version is bumped! |
# TODO (kai) add stricter types to block hamiltonian.jl#L37 from working on unknown metric/kinetic | ||
# The gradient of a position-dependent Hamiltonian system depends on both θ and r. | ||
∂H∂θ(h::Hamiltonian, θ::AbstractVecOrMat, r::AbstractVecOrMat) = ∂H∂θ(h, θ) | ||
∂H∂r(h::Hamiltonian, θ::AbstractVecOrMat, r::AbstractVecOrMat) = ∂H∂r(h, r) | ||
function ∂H∂r(h::Hamiltonian, θ::AbstractVecOrMat, r::AbstractVecOrMat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to remove these too.
AdvancedHMC.jl/src/riemannian/hamiltonian.jl
Lines 106 to 108 in 5a562e0
# TODO Make the order of θ and r consistent with neg_energy | |
∂H∂θ(h::Hamiltonian, θ::AbstractVecOrMat, r::AbstractVecOrMat) = ∂H∂θ(h, θ) | |
∂H∂r(h::Hamiltonian, θ::AbstractVecOrMat, r::AbstractVecOrMat) = ∂H∂r(h, r) |
@@ -72,9 +72,10 @@ function step( | |||
end | |||
#! Eq (17) of Girolami & Calderhead (2011) | |||
θ_full = copy(θ_init) | |||
term_1 = ∂H∂r(h, θ_init, r_half) # unchanged across the loop | |||
term_1 = ∂H∂r(h, θ_init, r_half).gradient # unchanged across the loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This computes neg_energy
but doesn't use it, which has performance implications. Maybe use a different name for functions that returns DualValue
, eg: neg_energy_and_∂H∂r
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, I suddenly realize the riemannian manifold HMC is not included in AdvancedHMC yet, this file is not included in the package, see
AdvancedHMC.jl/src/AdvancedHMC.jl
Line 50 in 5a562e0
include("riemannian/integrator.jl") |
I think we might start with #439, which is ready to get in now.
Fix: #173
While
∂H∂θ
return aDualValue
containing log density and its gradient, this PR makes∂H∂r
consistent with∂H∂θ
and return aDualValue
containing negative energy and momentum.This PR also change the weird argumnet order of
neg_energy(h, r, θ)
toneg_energy(h, θ, r)
, which is more commonly used in this package, e.g.phasepoint(h, θ, r)
,