Skip to content

Fix #411: Make rand_momentum part of the public API#488

Draft
nsiccha wants to merge 2 commits intomainfrom
issue-411-rand-momentum-api
Draft

Fix #411: Make rand_momentum part of the public API#488
nsiccha wants to merge 2 commits intomainfrom
issue-411-rand-momentum-api

Conversation

@nsiccha
Copy link
Copy Markdown
Contributor

@nsiccha nsiccha commented Mar 19, 2026

Look simple enough, but we may want to do more, see comments below.

Human content above


Claude content below

Make rand_momentum part of the public API

Problem

rand_momentum is an internal function used to sample momentum variables from a given metric and kinetic energy type. Packages that extend AdvancedHMC with custom metric types (e.g. Pathfinder.jl's RankUpdateEuclideanMetric) need to implement their own rand_momentum method, but because the function is not exported, they depend on an undocumented internal.

Change

One line added to src/AdvancedHMC.jl:

export UnitEuclideanMetric, DiagEuclideanMetric, DenseEuclideanMetric, rand_momentum

MWE

See 411-rand-momentum-api.jl. Checks that :rand_momentum ∈ names(AdvancedHMC) and that the function is callable. Fails on main with UndefVarError.

Notes

  • No behavior change — the function already exists and is fully implemented for all three built-in metric types plus the Riemannian metric.
  • Signature: rand_momentum(rng, metric, kinetic, θ) — the θ argument is ignored for Euclidean metrics but present for position-dependent (Riemannian) ones.
  • Maintainer already confirmed willingness to export in the issue comments.

MWE

411-rand-momentum-api.jl

# MWE for https://github.com/TuringLang/AdvancedHMC.jl/issues/411
# Fails on main (rand_momentum not exported), exits 0 after fix.

using AdvancedHMC, Random

# rand_momentum must appear in the public API (exported names)
if :rand_momentum  names(AdvancedHMC)
    @error "rand_momentum is not exported from AdvancedHMC"
    exit(1)
end

# Must also be callable
metric  = DiagEuclideanMetric(Float64, 2)
kinetic = GaussianKinetic()
r = rand_momentum(Random.default_rng(), metric, kinetic, zeros(2))
@assert r isa AbstractVector{Float64} && length(r) == 2

main output:

# MWE: 411-rand-momentum-api.jl on main
# started: 2026-03-19 20:01:33
# dir: /home/niko/github/issues/AdvancedHMC.jl
---
==> Pkg.instantiate()
==> Running 411-rand-momentum-api.jl
┌ Error: rand_momentum is not exported from AdvancedHMC
└ @ Main ~/github/issues/AdvancedHMC/proposals/411-rand-momentum-api.jl:8

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

worktree output:

# MWE: 411-rand-momentum-api.jl on worktree
# started: 2026-03-19 20:01:33
# dir: /home/niko/github/issues/AdvancedHMC/worktrees/issue-411-rand-momentum-api
---
==> Pkg.instantiate()
Precompiling packages...
    AdvancedHMC Being precompiled by another process (pid: 2096748, pidfile: /home/niko/.julia/compiled/v1.10/AdvancedHMC/WaglY_80WTF.ji.pidfile)
  10520.0 ms  ✓ AdvancedHMC
  1 dependency successfully precompiled in 17 seconds. 69 already precompiled.
==> Running 411-rand-momentum-api.jl

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

Fixes #411

rand_momentum is used by downstream packages (e.g. Pathfinder.jl) that
define custom metric types and need to implement their own rand_momentum
method. Exporting it avoids dependence on an undocumented internal.
@github-actions
Copy link
Copy Markdown
Contributor

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

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.71%. Comparing base (6bc0c74) to head (2dcdd66).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #488   +/-   ##
=======================================
  Coverage   77.71%   77.71%           
=======================================
  Files          21       21           
  Lines        1270     1270           
=======================================
  Hits          987      987           
  Misses        283      283           

☔ 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.

Copy link
Copy Markdown
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

I recognise that this is still draft, but I have found it good to always bump the version number and add a description of the change in HISTORY.md for every PR. This hasn't always been done in all TuringLang packages, but IMO it should be standard practice!

@nsiccha
Copy link
Copy Markdown
Contributor Author

nsiccha commented Mar 20, 2026

@penelopeysm yep! didn't we also at some point talk about having prs go into a dev branch first? and would we actually need additional tests if we expand the api? in principle yes, right? (in this particular case there may already be enough tests)

@nsiccha nsiccha changed the title Export rand_momentum as public API Fix #411: Make rand_momentum part of the public API Mar 20, 2026
@penelopeysm
Copy link
Copy Markdown
Member

Yeah, I wrote up a lot of this at https://turinglang.org/docs/contributing!

Ideally the tests would already have been present, but if not, it's never too soon to add some tests...

@nsiccha
Copy link
Copy Markdown
Contributor Author

nsiccha commented Mar 20, 2026

Or too late!

Per review feedback: add changelog entry and version bump for the
rand_momentum export.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.

Making rand_momentum part of the API

2 participants