refactor(ggm): single MH-move primitive shared by sampling and tuning#135
Merged
Conversation
The within-model edge and diagonal Metropolis moves were copy-pasted between the sampling path (update_edge_parameter/update_diagonal_parameter) and the Robbins-Monro tuning path (tune_proposal_sd) -- four RNG/math-identical blocks. Extract ggm_edge_move/ggm_diag_move, each proposing, computing the full acceptance ratio, applying the move on accept, and returning the RAW ln_alpha. The sampling wrappers return min(1,exp(ln_alpha)) (the accept prob consumed by do_one_metropolis_step); the tuning loops feed the raw ln_alpha to the Robbins-Monro update. Edge-active short-circuit and cache-flag invalidation stay at the callers. Proposal RNG and acceptance math now live in one place. Behavior-preserving: identical proposal/accept/update sequence, so the RNG draw order is unchanged. Verified bitwise -- adaptive-Metropolis GGM raw draws (no-edge delta=0, no-edge delta=1, edge-selection delta=0) digest-identical pre/post.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
+ Coverage 87.66% 87.69% +0.02%
==========================================
Files 87 87
Lines 12933 12897 -36
==========================================
- Hits 11338 11310 -28
+ Misses 1595 1587 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
What
The within-model edge and diagonal Metropolis moves were copy-pasted between the sampling path (
update_edge_parameter/update_diagonal_parameter) and the Robbins-Monro tuning path (tune_proposal_sd) — four RNG/math-identical blocks. This extracts two private primitives:ggm_edge_move(i, j)/ggm_diag_move(i)— propose, compute the full acceptance ratio, apply the move (with the Cholesky update) on accept, and return the rawln_alpha.The sampling wrappers return
min(1, exp(ln_alpha))(the accept probdo_one_metropolis_steprecords); the tuning loops feed the rawln_alphato the RM update. The edge-active short-circuit and cache-flag invalidation (constraint_dirty_/theta_valid_) stay at the callers; the edge-off path still returns0.0/continuebefore anyrnorm.Net: the proposal RNG and acceptance math live in one place;
tune_proposal_sdshrinks from two ~35-line bodies to two ~4-line calls.Why this is safe
Pure code extraction — identical proposal/accept/update sequence, so the RNG draw order is unchanged.
Verified bitwise: adaptive-Metropolis GGM raw draws (main + pairwise + indicator) are digest-identical pre/post across three scenarios that exercise every touched line:
delta = 0(within-K edge+diag moves + tuning, no tilt)delta = 1(adds thedeterminant_tilt_branch in both primitives)delta = 0(selection moves interleaved with within-K moves)identical(main, refactor) == TRUEfor all three. Harness:dev/bitwise_ggm_mh_primitive.R.