Skip to content

fix: diagonal-prior scale unification and full-space Jacobian#103

Merged
MaartenMarsman merged 3 commits into
mainfrom
hotfix/jacobian-and-diagonal-prior
May 15, 2026
Merged

fix: diagonal-prior scale unification and full-space Jacobian#103
MaartenMarsman merged 3 commits into
mainfrom
hotfix/jacobian-and-diagonal-prior

Conversation

@MaartenMarsman
Copy link
Copy Markdown
Collaborator

Summary

Two correctness fixes to the GGM gradient and prior code that were carried as a parked WIP for several weeks; same fixes ported to the mixed-MRF Kyy block.

Fix 1 — Diagonal-prior scale unification (GGM + mixed-MRF). The interaction prior already operated on the partial-association scale K_yy_ij = -K_ij/2, but the diagonal prior was evaluated at raw K_ii, so the two priors referred to different quantities. Every site now evaluates the diagonal prior at K_ii/2 (i.e. -K_yy_ii):

  • both NUTS gradient paths in GGMModel and MixedMRFModel
  • all MH ratios in GGMModel (update_edge_parameter, update_diagonal_parameter, both branches of update_edge_indicator_parameter_pair, tune_proposal_sd)
  • the continuous-block MH ratios in mixed_mrf_metropolis.cpp

The diagonal-prior adjoint becomes grad(K_ii/2) * Phi(:,i) after the chain-rule factor of 2 is absorbed.

User-visible effect: gamma_prior(shape, rate) is now interpreted as a prior on K_ii/2 rather than K_ii, so the implied mean of K_ii doubles. The default gamma_prior(1, 1) now corresponds to E[K_ii] = 2 rather than 1. The SBC prior simulator in test-sbc-ggm.R is rewritten to match.

Fix 2 — Full-space Jacobian aligned with theta-space (GGM). logp_and_gradient_full now uses the graph-agnostic Cholesky-to-K Jacobian
ldj = p*log(2) + 2*sum(psi) + sum_{k<p-1} (p-1-k)*psi_k
plus a per-column Pfaffian correction
pfaffian = 0.5 * sum_q log det(A_q diag(M_q^{-1}) A_q^T)
giving the correct RATTLE manifold-marginal under any diagonal mass. At identity mass the new full-space target agrees with theta-space at corresponding (theta, x). The Pfaffian adjoint flows back via two triangular solves through chol(G_q). The previous (deg_higher + 2) * psi_k form did not represent any RATTLE measure correction and conditioned on the active edge set incorrectly.

prior_test_interface.cpp gains an optional inv_mass_diag argument so the mass-weighted Pfaffian can be exercised from tests.

The matching mixed-MRF Fix 2 port is queued separately — the mixed-MRF Kyy block still carries the pre-fix Jacobian, and the projection helpers already build the required mass-weighted A_q matrices.

Test plan

  • devtools::load_all() rebuilds cleanly
  • devtools::test() passes locally
  • test-sbc-ggm.R SBC ranks remain uniform under the rewritten prior simulator
  • sample_ggm_prior diagonal-mean test hits the new 2 * shape/rate expectation
  • Finite-difference check on logp_and_gradient_full with random diagonal mass agrees with analytic gradient (~1e-6 per coordinate)
  • Theta-space vs full-space logp agreement at identity mass (~1e-10)

Two long-standing inconsistencies between the theta-space and full-space
gradient paths of GGMModel are resolved.

Fix 1: the diagonal prior is now evaluated on the partial-association
scale K_ii/2 (matching the interaction prior on -K_ij/2 and the mixed-MRF
continuous block), in both NUTS gradient paths and in all MH ratios in
GGMModel (update_edge_parameter, update_diagonal_parameter, both branches
of update_edge_indicator_parameter_pair, and tune_proposal_sd). The chain
rule absorbs the factor of two, so the diagonal-prior adjoint becomes
grad(K_ii/2) * Phi(:,i).

Fix 2: logp_and_gradient_full now uses the same graph-agnostic
Cholesky-to-K Jacobian as theta-space plus a per-column Pfaffian
correction
    pfaffian = 0.5 * sum_q log det(A_q diag(M_q^{-1}) A_q^T),
giving the correct RATTLE manifold-marginal under any diagonal mass.
At identity mass the new full-space target agrees with theta-space at
corresponding (theta, x). The Pfaffian adjoint flows back via two
triangular solves through chol(G_q).

prior_test_interface.cpp grows an optional inv_mass_diag argument so
the new mass-weighted Pfaffian can be exercised from tests; the SBC
prior simulator in test-sbc-ggm.R is rewritten to match the new K_ii/2
prior scale.
Mirrors Fix 1 from the GGM Kyy block in the mixed-MRF model's continuous
block: the diagonal prior is now applied to the partial-association
diagonal -K_yy_{ii} = Theta_{ii}/2 in both NUTS gradient paths
(logp_and_gradient, logp_and_gradient_full) and in the continuous-block
MH ratios in mixed_mrf_metropolis.cpp. Edge-indicator gating on the
off-diagonal slab is added in the same pass.

Fix 2 (graph-agnostic Cholesky-to-K Jacobian + per-column Pfaffian) is
queued separately; the mixed-MRF Kyy block still carries the pre-fix
(deg_higher_yy + 2) * psi_j Jacobian. The projection helpers in
mixed_mrf_model already build the mass-weighted A_q matrices, so the
per-column structure needed by Fix 2 is ready when that port lands.
Commit 1df5056 added inv_mass_diag to
ggm_test_logp_and_gradient_full_prior in prior_test_interface.cpp but
the auto-generated RcppExports.{R,cpp} were not regenerated; the symbol
mangle name no longer matched the definition, so the .so failed to load
at R CMD check time on CI.

Regenerated via Rcpp::compileAttributes(). Local devtools::test() now
passes 6989/6989.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

❌ Patch coverage is 76.92308% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.49%. Comparing base (2853ca5) to head (6499840).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/models/mixed/mixed_mrf_metropolis.cpp 36.00% 16 Missing ⚠️
src/models/ggm/ggm_gradient.cpp 90.16% 6 Missing ⚠️
src/models/ggm/ggm_model.cpp 76.47% 4 Missing ⚠️
src/prior_test_interface.cpp 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
- Coverage   87.00%   86.49%   -0.52%     
==========================================
  Files          77       77              
  Lines       13146    13267     +121     
==========================================
+ Hits        11438    11475      +37     
- Misses       1708     1792      +84     

☔ 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 15, 2026 05:45
@MaartenMarsman MaartenMarsman merged commit 5988230 into main May 15, 2026
8 of 10 checks passed
@MaartenMarsman MaartenMarsman deleted the hotfix/jacobian-and-diagonal-prior branch May 15, 2026 05:45
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