Fix/marginal pl correctness#97
Merged
Merged
Conversation
Three independent corrections to GGMModel:
1. Likelihood degrees of freedom: centered observations have n-1 effective
degrees of freedom, not n. Previously caused systematic overestimation
of diagonal precision elements.
2. Guard initialize_precision_from_mle() against n=0 so prior-only sampling
doesn't divide by zero in the regularizer delta = trace(S) / (p * n).
3. Remove three redundant Gamma(1, 1) priors on the constrained Cholesky
diagonal K_jj (in update_edge_parameter and update_edge_indicator_pair).
Under the constrained parameterization the diagonal is a deterministic
function of phi_{q-1,q} with phi_{q,q} fixed, so the Gamma(1,1) prior
cancels in the Hastings ratio.
Analogous to the standalone GGM fixes from commit 18f4e39 on
feature/prior-classes; the prior-classes-specific bits of that commit
(InteractionPriorType wiring, sample_ggm_prior infrastructure, doc
updates for interaction_prior / threshold_prior args) stay with
prior-classes.
The off-diagonal cross-induced contribution to the effective interaction
matrix M = A_xx + 2 A_xy Σ_yy A_xy' was under-counted by a factor of 2 in
the rest-score computation, so the analytic gradient disagreed with
finite differences. Forward log-posterior and analytic gradient now match
a direct Gaussian-integral reference and central FD to machine precision.
Three coordinated fixes:
1. recompute_marginal_interactions(): drop the spurious factor 2 on
pairwise_effects_discrete_. M is now A_xx + 2 A_xy Σ A_xy', not
2 A_xx + 2 A_xy Σ A_xy'. The factor 2 from the x'Mx derivative is
carried at the call site, not folded into M.
2. log_marginal_omrf() and the analogous gradient code paths: the rest
score now multiplies (M · x - self · M_ss) by 2.0, mirroring the
conditional PL structure where the rest score is 2 · A_xx · x_{-s}.
3. Gradient chain-rule factors: every site that propagates back through
M has its multiplier doubled where appropriate:
- Theta_bar(s, t) off-diagonal: factor 1 → 2
- cross_self first term: factor 2 → 4
- V_s for cross-contribution to other variables: factor 2 → 4
- logp numerator self-quadratic: temp_marginal(s,s) * x_s² →
precision_ss * x_s² (cosmetic, identical value)
Adapted from the corresponding Part 2 of commit 6853774 on
feature/prior-classes. The prior-classes version applied these inside
`if (use_marginal_pl_) { ... }` branches; on main those branches were
collapsed by PR #94 (cleanup conditional pseudo-likelihood), so the
fixes now apply unconditionally.
The factor-of-2 bug never shipped — it lived only in the 0.2.0.0
development branch — but it would have produced incorrect posteriors for
mixed-MRF fits using NUTS or adaptive Metropolis.
NEWS.md updated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
+ Coverage 87.13% 87.14% +0.01%
==========================================
Files 72 72
Lines 12294 12292 -2
==========================================
Hits 10712 10712
+ Misses 1582 1580 -2 ☔ 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.
Fix/marginal pl correctness