Chore/sampler rename and collapse#95
Merged
Merged
Conversation
Pure HMC with a fixed trajectory length is superseded by NUTS, which
adapts trajectory length and is more reliable on edge-selected GGMs
(where pure HMC's RATTLE integration is fragile). Both arguments were
already deprecated; this removes them entirely along with the supporting
machinery.
Removed:
- src/mcmc/samplers/hmc_sampler.h (HMCSampler class)
- update_hmc_bgmcompare() in src/bgmCompare/bgmCompare_sampler.cpp
- hamiltonian_mc enum value and string-parser branch
- num_leapfrogs field on SamplerConfig and dead pass-through in
sample_omrf / sample_mixed_mrf entry points
- hmc_num_leapfrogs argument throughout the R API and bgmCompare's
C++ Rcpp interface
Kept (NUTS continues to use them):
- src/mcmc/algorithms/{hmc,leapfrog}.* (Hamiltonian / leapfrog / RATTLE
primitives)
- src/mcmc/samplers/hmc_adaptation.h (step-size + mass-matrix controller)
User-facing impact: bgm()/bgmCompare() with update_method = "hamiltonian-mc"
or hmc_num_leapfrogs = ... now error out (match.arg / unused argument).
NEWS.md updated. HMC compliance fixtures and the test-hmc-ggm.R suite
deleted; helper-fixtures.R no longer exposes get_bgms_fit_hmc().
All 6272 tests pass. Smoke tests confirm clean rejection of removed args.
The marginal pseudo-likelihood (which integrates out the continuous block when computing discrete full conditionals) is the principled default for mixed MRF models. The conditional pseudo-likelihood — which conditioned on observed continuous values — was a legacy approximation that the recent fix to the mixed-MRF marginal PL target (commit 6853774) made obsolete. This removes the dual-mode plumbing entirely; marginal is now the only mode. Removed: - MixedMRFModel constructor parameter `pseudolikelihood` and the `use_marginal_pl_` member, plus the conditional initializer - ~13 dual-mode branches in mixed_mrf_metropolis.cpp (collapsed to marginal) - ~19 dual-mode branches in mixed_mrf_gradient.cpp (collapsed to marginal) - log_conditional_omrf() — no callers remained after the collapse - pseudolikelihood argument from R API (bgm, bgm_spec) and from C++ Rcpp interfaces (sample_mixed_mrf, mixed_test_logp_and_gradient[_full], mixed_test_project_position, mixed_test_project_momentum, mixed_test_leapfrog_constrained) - pseudolikelihood from tests (test-mixed-nuts, test-mixed-gradient, test-mixed-rattle, test-mixed-leapfrog, test-scaling-diagnostics, test-validation-slow); the conditional-vs-marginal comparison test was deleted (no longer meaningful) Kept (still used elsewhere or describe single-node densities): - log_marginal_omrf() — now the only OMRF likelihood path - log_conditional_ggm() — used by all GGM-related updates regardless of mode - recompute_marginal_interactions() — now called unconditionally - marginal_interactions_ member — now always allocated User-facing impact: bgm() with pseudolikelihood = "..." now errors with "unused argument". Mixed-MRF fits previously requesting "conditional" will now silently use marginal — semantically a more correct default but a behavior change for any caller relying on the conditional approximation. NEWS.md updated. All 6272 tests pass. Smoke tests confirm the removed argument is rejected and that NUTS / adaptive-metropolis on mixed-MRF still produce finite output.
After pure HMC removal, the warmup adaptation controller is used only by
NUTS. Rename it to match.
- src/mcmc/samplers/hmc_adaptation.h → nuts_adaptation.h
- HMCAdaptationController → NUTSAdaptationController
- hmc_adapt → nuts_adapt at all call sites (bgmCompare's NUTS path)
- Replace "HMC/NUTS" / "NUTS/HMC" / "HMC and NUTS" mentions with "NUTS"
in sampler/model headers and comments. Algorithm-level mentions in
src/mcmc/algorithms/{hmc,leapfrog}.h are preserved — those describe
the underlying Hamiltonian dynamics that NUTS still uses.
No behavior change; renames only.
GradientSamplerBase was originally introduced to share warmup adaptation between HMCSampler and NUTSSampler. With pure HMC removed, NUTS is the only descendant. The intermediate base class is dead weight: it adds an inheritance layer with no payoff and uses misleading naming (the "shared base for gradient-based MCMC" now serves a single sampler). This commit folds the gradient scaffolding directly into NUTSSampler: - step() now contains the Stage-3c restart, mass-matrix update orchestration, step-size heuristic re-run, and the leapfrog/RATTLE call as one body. - initialize() / do_initialize() and the NUTSAdaptationController member (renamed nuts_adapt_) move into NUTSSampler as private members. - The protected do_gradient_step() virtual hook goes away; NUTSSampler dispatches to do_unconstrained_step() / do_constrained_step() inline. Hierarchy is now flat: SamplerBase (abstract: step, initialize, has_nuts_diagnostics) ├── MetropolisSampler (forwards to model.do_one_metropolis_step) └── NUTSSampler (gradient scaffolding + leapfrog/RATTLE) If a second gradient sampler returns later, re-introducing the base class is mechanical. Per the design notes in dev/plans/backlog/post-hmc-sampler-cleanup.md, no symmetric do_step() hook is added: MetropolisSampler has nothing to scaffold, and the NUTS adaptation controller's per-sampler ownership is principled (not symmetric with the model-side MetropolisAdaptationControllers). src/models/base_model.h doc comments updated: "Used by GradientSamplerBase" → "Used by NUTSSampler". No behavior change; restructure only. All 6272 tests pass.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
==========================================
- Coverage 87.17% 87.05% -0.12%
==========================================
Files 71 71
Lines 12255 12252 -3
==========================================
- Hits 10683 10666 -17
- Misses 1572 1586 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MaartenMarsman
added a commit
that referenced
this pull request
Apr 27, 2026
Brings the HMC removal (PR #93), conditional pseudolikelihood removal (PR #94), sampler rename/collapse (PR #95), NUTS Stan-compliance fixes (PR #96), and marginal-PL correctness fixes (PR #97) into the prior-classes branch. Conflict resolution rule: - HMC code → take main's deletion (drops update_hmc_bgmcompare, hamiltonian_mc enum branches, hmc_num_leapfrogs args, .Deprecated paths, HMC test fixtures) - if(use_marginal_pl_) { marginal } else { conditional } → keep marginal branch only; drop the use_marginal_pl_ field and pseudolikelihood string parameter from MixedMRFModel constructor and downstream test interfaces - Prior-class additions (interaction_prior_, threshold_prior_, means_prior_, diagonal_prior_) preserved everywhere Test helper updated to inject the prior-class C++ test functions (test_parameter_prior, test_scale_prior, ggm_test_logp_and_gradient_prior, ggm_test_logp_and_gradient_full_prior, ggm_test_leapfrog_constrained_checked, unpack_interaction_prior, unpack_threshold_prior) into globalenv when running outside R CMD check. test-mixed-mrf-likelihood.R reverted to main's version (the marginal-vs-conditional comparison test no longer applies after conditional PL removal). 6663 / 6732 tests pass, 0 fail, 0 error, 69 skip (slow/CRAN-gated).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two-commit cleanup of the sampler hierarchy after pure HMC removal:
HMCAdaptationController→NUTSAdaptationController,hmc_adaptation.h→nuts_adaptation.h,hmc_adapt→nuts_adapt, comment cleanup of "HMC/NUTS" co-mentions in sampler/model headers. Algorithm-level mentions (mcmc/algorithms/hmc.h,leapfrog.h) preserved — those describe Hamiltonian dynamics that NUTS still uses.GradientSamplerBaseis now empty (single descendant after HMC removal); folded its scaffolding directly intoNUTSSampler::step()/initialize(). Thedo_gradient_step()virtual hook goes away.New hierarchy:
SamplerBase
├── MetropolisSampler (forwards to model.do_one_metropolis_step)
└── NUTSSampler (gradient scaffolding + leapfrog/RATTLE)
No symmetric
do_step()hook —MetropolisSamplerhas nothing to scaffold, and the NUTS adaptation controller's per-sampler ownership is principled (asymmetric with the model-sideMetropolisAdaptationControllers on purpose). Rationale captured indev/plans/backlog/post-hmc-sampler-cleanup.md.Test plan
roxygen2::roxygenise()— no-opRcpp::compileAttributes()— no-opstyler::style_pkg()idempotentlintr::lint_package()— no lintsdevtools::test()→ 6272 PASS / 0 FAIL / 68 SKIP