Skip to content

Fix #483: Rename neg_energyneg_kinetic_energy#490

Draft
nsiccha wants to merge 1 commit intomainfrom
issue-483-rename-neg-energy
Draft

Fix #483: Rename neg_energyneg_kinetic_energy#490
nsiccha wants to merge 1 commit intomainfrom
issue-483-rename-neg-energy

Conversation

@nsiccha
Copy link
Copy Markdown
Contributor

@nsiccha nsiccha commented Mar 19, 2026

I'll check the changes before marking it as ready for review.

Human content above


Claude content below

Rename neg_energyneg_kinetic_energy

Problem

neg_energy is overloaded with three distinct meanings:

  • neg_energy(z::PhasePoint) → total negative Hamiltonian energy (ℓπ + ℓκ)
  • neg_energy(h, θ) → negative potential energy (log density)
  • neg_energy(h, r, θ) → negative kinetic energy

The third overload (the one taking momentum r) is the confusing one: internally it is cached as ℓκ (the "neg kinetic energy") in PhasePoint, and the Riemannian code even has a comment # Negative kinetic energy directly above its definition. The naming mismatch makes it hard to understand what phasepoint(h, θ, r) is actually caching.

Change

4 files changed (21 insertions, 20 deletions):

  • src/hamiltonian.jl — rename 5 neg_energy(h, r, θ) definitions to neg_kinetic_energy; update the two phasepoint call sites; add explicit energy(h, r, θ) dispatch that routes to neg_kinetic_energy (the energy(args...) catch-all can no longer reach the renamed function)
  • src/riemannian/hamiltonian.jl — rename the 6th kinetic overload (Riemannian metric); update import and phasepoint call site
  • test/hamiltonian.jl — update 6 test assertions
  • test/riemannian.jl — update import and 1 assertion

MWE

See 483-rename-neg-kinetic-energy.jl. Checks that AdvancedHMC.neg_kinetic_energy is defined and callable, and that neg_energy(z::PhasePoint) still works. Fails on main because neg_kinetic_energy does not exist.

Notes

  • neg_energy(z::PhasePoint) and neg_energy(h, θ) are untouched — only the kinetic-energy-specific overloads are renamed.
  • The new energy(h, r, θ) dispatch preserves the existing energy interface used in test/riemannian.jl.
  • All functions remain internal (not exported).

MWE

483-rename-neg-kinetic-energy.jl

# MWE for https://github.com/TuringLang/AdvancedHMC.jl/issues/483
# Fails on main (neg_kinetic_energy does not exist), exits 0 after fix.

using AdvancedHMC, LinearAlgebra

h = Hamiltonian(
    DiagEuclideanMetric(Float64, 2),
    x -> -sum(abs2, x) / 2,
    x -> (-sum(abs2, x) / 2, -x),
)
r, θ = randn(2), randn(2)

# neg_kinetic_energy must exist after the rename
if !isdefined(AdvancedHMC, :neg_kinetic_energy)
    @error "neg_kinetic_energy is not defined in AdvancedHMC"
    exit(1)
end

val = AdvancedHMC.neg_kinetic_energy(h, r, θ)
@assert val isa Real

# The total-energy overload on PhasePoint must still work unchanged
z = AdvancedHMC.phasepoint(h, θ, r)
@assert AdvancedHMC.neg_energy(z) isa Real

main output:

# MWE: 483-rename-neg-kinetic-energy.jl on main
# started: 2026-03-19 20:01:32
# dir: /home/niko/github/issues/AdvancedHMC.jl
---
==> Pkg.instantiate()
==> Running 483-rename-neg-kinetic-energy.jl
┌ Error: neg_kinetic_energy is not defined in AdvancedHMC
└ @ Main ~/github/issues/AdvancedHMC/proposals/483-rename-neg-kinetic-energy.jl:15

# exit_code: 1
# status: FAIL
# finished: 2026-03-19 20:01:53

worktree output:

# MWE: 483-rename-neg-kinetic-energy.jl on worktree
# started: 2026-03-19 20:01:32
# dir: /home/niko/github/issues/AdvancedHMC/worktrees/issue-483-rename-neg-energy
---
==> Pkg.instantiate()
Precompiling packages...
    AdvancedHMC Being precompiled by another process (pid: 2096748, pidfile: /home/niko/.julia/compiled/v1.10/AdvancedHMC/WaglY_80WTF.ji.pidfile)
  31008.7 ms  ✓ AdvancedHMC
  1 dependency successfully precompiled in 35 seconds. 69 already precompiled.
==> Running 483-rename-neg-kinetic-energy.jl

# exit_code: 0
# status: PASS
# finished: 2026-03-19 20:02:18

Fixes #483

Rename the kinetic-energy-specific neg_energy overloads (those taking
momentum r) to neg_kinetic_energy to eliminate the naming ambiguity:

- neg_energy(z::PhasePoint) → total negative Hamiltonian (unchanged)
- neg_energy(h, θ)          → negative potential energy (unchanged)
- neg_kinetic_energy(h,r,θ) → negative kinetic energy (renamed)

Add an explicit energy(h, r, θ) = -neg_kinetic_energy(h, r, θ) dispatch
so the energy interface used in tests continues to work after the rename.

4 files changed: src/hamiltonian.jl, src/riemannian/hamiltonian.jl,
test/hamiltonian.jl, test/riemannian.jl.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

function phasepoint(
h::Hamiltonian, θ::T, r::T; ℓπ=∂H∂θ(h, θ), ℓκ=DualValue(neg_energy(h, r, θ), ∂H∂r(h, r))
h::Hamiltonian, θ::T, r::T; ℓπ=∂H∂θ(h, θ), ℓκ=DualValue(neg_kinetic_energy(h, r, θ), ∂H∂r(h, r))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
h::Hamiltonian, θ::T, r::T; ℓπ=∂H∂θ(h, θ), ℓκ=DualValue(neg_kinetic_energy(h, r, θ), ∂H∂r(h, r))
h::Hamiltonian,
θ::T,
r::T;
ℓπ=∂H∂θ(h, θ),
ℓκ=DualValue(neg_kinetic_energy(h, r, θ), ∂H∂r(h, r)),

end

energy(args...) = -neg_energy(args...)
energy(h::Hamiltonian, r::AbstractVecOrMat, θ::AbstractVecOrMat) = -neg_kinetic_energy(h, r, θ)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
energy(h::Hamiltonian, r::AbstractVecOrMat, θ::AbstractVecOrMat) = -neg_kinetic_energy(h, r, θ)
function energy(h::Hamiltonian, r::AbstractVecOrMat, θ::AbstractVecOrMat)
-neg_kinetic_energy(h, r, θ)
end


h = Hamiltonian(UnitEuclideanMetric(T, D), ℓπ, ∂ℓπ∂θ)
@test -AdvancedHMC.neg_energy(h, r_init, θ_init) == sum(abs2, r_init) / 2
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) == sum(abs2, r_init) / 2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) == sum(abs2, r_init) / 2
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) ==
sum(abs2, r_init) / 2

M⁻¹ = m' * m
h = Hamiltonian(DenseEuclideanMetric(M⁻¹), ℓπ, ∂ℓπ∂θ)
@test -AdvancedHMC.neg_energy(h, r_init, θ_init) ≈ r_init' * M⁻¹ * r_init / 2
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) ≈ r_init' * M⁻¹ * r_init / 2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) r_init' * M⁻¹ * r_init / 2
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init)
r_init' * M⁻¹ * r_init / 2


h = Hamiltonian(UnitEuclideanMetric(T, 2 * D), ℓπ, ∂ℓπ∂θ)
@test -AdvancedHMC.neg_energy(h, r_init, θ_init) == sum(abs2, r_init) / 2
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) == sum(abs2, r_init) / 2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) == sum(abs2, r_init) / 2
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) ==
sum(abs2, r_init) / 2

M⁻¹ = ComponentArray(m' * m, ax, ax)
h = Hamiltonian(DenseEuclideanMetric(M⁻¹), ℓπ, ∂ℓπ∂θ)
@test -AdvancedHMC.neg_energy(h, r_init, θ_init) ≈ r_init' * M⁻¹ * r_init / 2
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) ≈ r_init' * M⁻¹ * r_init / 2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init) r_init' * M⁻¹ * r_init / 2
@test -AdvancedHMC.neg_kinetic_energy(h, r_init, θ_init)
r_init' * M⁻¹ * r_init / 2

@testset "Kinetic energy" begin
Σ = hamiltonian.metric.map(hamiltonian.metric.G(x))
@test neg_energy(hamiltonian, r, x) ≈ logpdf(MvNormal(zeros(D), Σ), r)
@test neg_kinetic_energy(hamiltonian, r, x) ≈ logpdf(MvNormal(zeros(D), Σ), r)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@test neg_kinetic_energy(hamiltonian, r, x) logpdf(MvNormal(zeros(D), Σ), r)
@test neg_kinetic_energy(hamiltonian, r, x)
logpdf(MvNormal(zeros(D), Σ), r)

@github-actions
Copy link
Copy Markdown
Contributor

AdvancedHMC.jl documentation for PR #490 is available at:
https://TuringLang.github.io/AdvancedHMC.jl/previews/PR490/

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.65%. Comparing base (6bc0c74) to head (fccf80b).

Files with missing lines Patch % Lines
src/hamiltonian.jl 83.33% 1 Missing ⚠️
src/riemannian/hamiltonian.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
- Coverage   77.71%   77.65%   -0.07%     
==========================================
  Files          21       21              
  Lines        1270     1271       +1     
==========================================
  Hits          987      987              
- Misses        283      284       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nsiccha nsiccha changed the title Fix #483: rename neg_energy(h,r,θ) → neg_kinetic_energy Fix #483: Rename neg_energyneg_kinetic_energy Mar 20, 2026
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.

Internal function neg_energy should be called neg_kinetic_energy

1 participant