Skip to content

Fix missing MH proposal-SD adaptation under NUTS for GGM and MixedMRF#102

Merged
MaartenMarsman merged 1 commit into
mainfrom
refactor/align-warmup-adaptation
May 14, 2026
Merged

Fix missing MH proposal-SD adaptation under NUTS for GGM and MixedMRF#102
MaartenMarsman merged 1 commit into
mainfrom
refactor/align-warmup-adaptation

Conversation

@MaartenMarsman
Copy link
Copy Markdown
Collaborator

Description

Fix missing MH proposal-SD adaptation under NUTS for GGM and MixedMRF, and align the adaptation machinery across all three concrete models (OMRF, GGM, MixedMRF) so that the bug can't recur silently.

Problem / Motivation

On main, GGM and MixedMRF chains under update_method = "nuts" never adapt the between-model MH proposal SDs. The values used to propose new continuous-parameter values when an edge toggles ON stay at their initial setting for the entire run. The mechanics:

  • MixedMRF's tune_proposal_sd is a stub on main — its body is a comment saying "RM is embedded in update_*".
  • The inline RM in those update_* functions only fires when those functions are called by MetropolisSampler::step, which is the MH path, not NUTS.
  • Under NUTS, NUTS samples the continuous parameters; the model's update_* functions never run; the inline RM never fires; the proposal SDs stay at their initial value.
  • The result: trans-dimensional edge-toggle proposals (which use those SDs to draw a fresh continuous value when an edge flips ON) are under-mixed for the entire NUTS run. Edge-add acceptance is biased low for large true effects and the chain has a harder time finding them.

A second issue, smaller but on the same path, was uncovered during the fix:

  • Under NUTS, validate_sampler.R defaults target_accept to 0.80 (HMC dual-averaging optimum). That value was forwarded to set_metropolis_target_accept, so during stage-3b RM the MH SDs were being tuned to AR ≈ 0.80 instead of the componentwise RW MH optimum 0.44.

Proposed Changes / New Functionality

  1. Wire stage-3b proposal-SD adaptation for GGM and MixedMRF under NUTS.

    • MixedMRF's tune_proposal_sd now performs an actual within-model MH sweep with RM weights, not a stub.
    • Both models gain a MetropolisAdaptationController (one for GGM, five for MixedMRF — one per proposal-SD storage), matching OMRF's pattern. Under MH the controller does per-iteration RM; under NUTS it stays null and stage-3b is the adaptation path.
    • Each update_* function now returns its per-slot Metropolis acceptance probability (double instead of void). do_one_metropolis_step collects them into matrices and calls the adapter(s) once per iteration.
  2. Decouple MH and NUTS targets. In sample_{omrf,ggm,mixed}.cpp, under sampler_type == "nuts" the user's target_accept continues to drive HMC dual averaging unchanged, but set_metropolis_target_accept is called with hardcoded 0.44 so the between-model MH proposal SDs target their own optimum.

  3. Consolidate the stage-3b RM weight policy. WarmupSchedule::rm_weight_for_proposal_sd(iter) is now the single source of truth for the weight schedule and decay rate (proposal_sd_rm_decay = 0.75). Every model's tune_proposal_sd reads it; nobody computes the weight inline.

  4. Storage shape. GGM proposal_sds_ and MixedMRF proposal_sd_main_continuous_ switched from arma::vec to (N, 1) arma::mat so the controller can hold an arma::mat& reference directly.

Type of Change

  • Bug fix (missing proposal-SD adaptation under NUTS for GGM and MixedMRF; NUTS step-size target leaking into MH RM)
  • New feature
  • Breaking change
  • Documentation update
  • Performance optimization
  • Statistical / methodological update (correct RM target for componentwise RW MH; trans-dimensional edge-toggle proposals now tuned under NUTS)

Documentation and Release Notes

  • Updated function documentation for any signature or behaviour changes (Doxygen on changed update_*, init_metropolis_adaptation, tune_proposal_sd, sweep_within_model_mh)
  • Regenerated man/*.Rd files if roxygen comments changed (no R-side roxygen changed)
  • Added or updated NEWS.md entry if the change affects users (needs an entry — see Additional Notes)

Testing and Validation

Verified that:

  1. OMRF/GGM/MixedMRF MH samples are bitwise identical to main (seed-fixed runs; main, pairwise, and indicator matrices all match exactly). The MH path is not affected by the NUTS fix.

  2. MH and NUTS algorithmic paths agree within each model — per-parameter posterior-mean correlations:

    model main mean pair mean ind mean
    OMRF 0.9999 0.9986 0.9966
    GGM 0.9993 1.0000 1.0000
    MixedMRF 1.0000 0.9999 1.0000
  3. Stage-3b realized acceptance rate converges near 0.44 (target) for all three models post-fix, vs ~0.78 (the wrong NUTS step-size target 0.80) pre-fix.

Checks run:

  • Added or updated tests in tests/testthat/ when needed (no new R-level surface; existing testthat suite passes)
  • Ran the project styler (0 files changed)
  • Ran lintr::lint_package() (0 findings)
  • Ran roxygen2::roxygenise() (no man/*.Rd changes)
  • Ran rcmdcheck::rcmdcheck(args = c("--no-manual", "--as-cran")) (0 errors, 0 test failures; 1 warning re missing checkbashisms local utility, 1 NOTE re stale DESCRIPTION Date — neither code-related)
  • Verified numerical behaviour (bitwise MH equivalence + MH↔NUTS posterior agreement + stage-3b AR convergence)
    • For all three models, the between-model MH proposal SDs now target AR = 0.44 instead of inheriting the user's NUTS step-size target (default 0.80). SDs converge to wider values.
  • Follow-up deferred (dev/plans/backlog/align-adaptive-metropolis-path.md): OMRF retains both MetropolisAdaptationController (per-iteration, MH mode) and the stage-3b path; GGM/MixedMRF now mirror this. Stage 3b is the only place the trans-dimensional edge-toggle proposal SDs get tuned under NUTS — NUTS cannot perform the between-model jump itself — so the two paths serve genuinely different roles. The question is whether MetropolisAdaptationController should also fire during stage 3b under NUTS (or vice versa), not whether stage 3b is redundant.

- Centralize stage-3b RM weight in WarmupSchedule::rm_weight_for_proposal_sd.
- Wire GGM and MixedMRF MH paths to MetropolisAdaptationController, mirroring
  OMRF. update_* return per-slot accept_prob; do_one_metropolis_step collects
  them and calls the adapter(s) once per iteration.
- Decouple NUTS step-size target (default 0.80) from MH componentwise RW MH
  target (0.44) in sample_{omrf,ggm,mixed}.cpp.
- Switch GGM proposal_sds_ and MixedMRF proposal_sd_main_continuous_ from
  arma::vec to (N, 1) arma::mat so the adapter can hold an arma::mat&.

OMRF/GGM/MixedMRF MH samples bitwise identical to main; MH vs NUTS
per-parameter posterior means agree across all three models (cor > 0.999).
@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

❌ Patch coverage is 14.70588% with 116 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.58%. Comparing base (2853ca5) to head (23c3dec).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/models/mixed/mixed_mrf_model.cpp 5.26% 72 Missing ⚠️
src/models/mixed/mixed_mrf_metropolis.cpp 0.00% 24 Missing ⚠️
src/models/ggm/ggm_model.cpp 13.04% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
- Coverage   87.00%   86.58%   -0.43%     
==========================================
  Files          77       77              
  Lines       13146    13217      +71     
==========================================
+ Hits        11438    11444       +6     
- Misses       1708     1773      +65     

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

@MaartenMarsman MaartenMarsman marked this pull request as ready for review May 14, 2026 20:28
@MaartenMarsman MaartenMarsman merged commit 20e7662 into main May 14, 2026
8 of 10 checks passed
@MaartenMarsman MaartenMarsman deleted the refactor/align-warmup-adaptation branch May 14, 2026 20:28
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.

1 participant