Skip to content

Making rand_momentum part of the API #411

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

Open
sethaxen opened this issue Mar 24, 2025 · 2 comments · May be fixed by #443
Open

Making rand_momentum part of the API #411

sethaxen opened this issue Mar 24, 2025 · 2 comments · May be fixed by #443

Comments

@sethaxen
Copy link
Member

As instructed by the v0.7.0 release notes, I'm opening this issue to propose making rand_momentum part of the API.

Pathfinder.jl provides an approximation of a target distribution as a multivariate normal whose covariance matrix has a particular structure that can be exploited for efficient sampling. Pathfinder implements RankUpdateEuclideanMetric here, which allows this covariance matrix to be used as an (inverse) metric in AdvancedHMC (for usage examples, see https://mlcolab.github.io/Pathfinder.jl/v0.9/examples/initializing-hmc/#AdvancedHMC.jl)

The recent replacement of rand methods with internal rand_momentum methods require Pathfinder to implement this internal method to support integration with AdvancedHMC v0.7.0. It would be useful if rand_momentum was added to the public API to maker overloading it safer.

Alternatives:

  • The proposal in Simplifying Metric types by using AbstractPDMat #34 to implement a single Metric object wrapping an AbstractPDMat would also work, since Pathfinder's covariance matrix type is also an AbstractPDMat.
  • RankUpdateEuclideanMetric could live here in an extension instead of Pathfinder. However, since Pathfinder is more specific than this package, I think that makes less sense.
  • One could blend the two alternatives above by adding a EuclideanMetric struct that wraps an AbstractPDMat and allows any PDMat to be wrapped as a metric that implements the necessary interface. It would be good if this could support wrapping either the inverse metric or the metric itself. For the former, doing this efficiently and generically requires some package (maybe PDMats) implements something like a sqrt_fact function that returns a factor R such that R' * R is the PDMat and R implement ldiv!. This generalizes the Cholesky factorization for structured matrices for which a different square root factorization may be more efficient to compute and use than a Cholesky factorization.
@yebai
Copy link
Member

yebai commented Mar 24, 2025

Happy to make rand_momentum public -- do you want to create a PR?

@sethaxen
Copy link
Member Author

Sure!

@ErikQQY ErikQQY linked a pull request May 9, 2025 that will close this issue
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 a pull request may close this issue.

2 participants