-
Notifications
You must be signed in to change notification settings - Fork 229
added requested docstrings #2692
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
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.
Pull Request Overview
Adds and improves documentation for adaptive Hamiltonian sampling in Turing, addressing requested docstrings and minor formatting.
- Add comprehensive docstring for AbstractMCMC.sample with AdaptiveHamiltonian, detailing adaptation/warmup controls.
- Add docstring for DynamicPPL.initialstep to clarify initialization behavior and keywords.
- Normalize NUTS docstring section header to “# Arguments”.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/mcmc/hmc.jl
Outdated
| ```julia | ||
| NUTS() # Use default NUTS configuration. | ||
| NUTS(1000, 0.65) # Use 1000 adaption steps, and target accept ratio 0.65. |
Copilot
AI
Oct 20, 2025
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.
Correct 'adaption' to 'adaptation' for clarity and consistency.
| NUTS(1000, 0.65) # Use 1000 adaption steps, and target accept ratio 0.65. | |
| NUTS(1000, 0.65) # Use 1000 adaptation steps, and target accept ratio 0.65. |
Copilot uses AI. Check for mistakes.
src/mcmc/hmc.jl
Outdated
| """ | ||
| struct NUTS{AD,metricT<:AHMC.AbstractMetric} <: AdaptiveHamiltonian | ||
| n_adapts::Int # number of samples with adaption for ϵ |
Copilot
AI
Oct 20, 2025
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.
Correct 'adaption' to 'adaptation' in the field comment.
| n_adapts::Int # number of samples with adaption for ϵ | |
| n_adapts::Int # number of samples with adaptation for ϵ |
Copilot uses AI. Check for mistakes.
|
Turing.jl documentation for PR #2692 is available at: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2692 +/- ##
=======================================
Coverage 87.21% 87.21%
=======================================
Files 22 22
Lines 1431 1431
=======================================
Hits 1248 1248
Misses 183 183 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/mcmc/hmc.jl
Outdated
| - `initial_params`: Initial parameter values for sampling. See `DynamicPPL.initialstep` for details. | ||
| Additional keyword arguments are passed to the underlying sampling implementation. |
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.
Are all the other keyword arguments specific to HMC, or do they include generic ones joint to all samplers, like verbose or something?
Relatedly, may be worth linking to this in the docstring: https://turinglang.org/docs/usage/sampling-options Maybe already above?
src/mcmc/hmc.jl
Outdated
| # Keyword Arguments | ||
| - `initial_params`: Initial parameter values to use for sampling. If `nothing` (the default), |
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.
initial_params and verbose work the same way for all samplers, not just HMC, so I would not document them here, but rather refer to the docstring of the more generic initialstep function or method. That docstring may need writing too, but would make sense to postpone that after github.com//pull/2676, because the way initial_params works is going to change quite drastically.
7fd1640 to
152b190
Compare
- Improved docstring for initial_params to reference DynamicPPL.initialstep - Added link to sampling options documentation for keyword arguments - Fixed typos: "adaption" → "adaptation" throughout HMCDA and NUTS
152b190 to
1887fff
Compare
WRT: #2684