Fix #411: Make rand_momentum part of the public API#488
Fix #411: Make rand_momentum part of the public API#488
rand_momentum part of the public API#488Conversation
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.
|
AdvancedHMC.jl documentation for PR #488 is available at: |
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
penelopeysm
left a comment
There was a problem hiding this comment.
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!
|
@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) |
rand_momentum part of the public API
|
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... |
|
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>
Look simple enough, but we may want to do more, see comments below.
Human content above
Claude content below
Make
rand_momentumpart of the public APIProblem
rand_momentumis 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'sRankUpdateEuclideanMetric) need to implement their ownrand_momentummethod, 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_momentumMWE
See
411-rand-momentum-api.jl. Checks that:rand_momentum ∈ names(AdvancedHMC)and that the function is callable. Fails onmainwithUndefVarError.Notes
rand_momentum(rng, metric, kinetic, θ)— theθargument is ignored for Euclidean metrics but present for position-dependent (Riemannian) ones.MWE
411-rand-momentum-api.jlmain output:
worktree output:
Fixes #411