Skip to content

refactor(s5): share GGM/Mixed precision kernels as cholesky_helpers free functions#140

Merged
MaartenMarsman merged 1 commit into
mainfrom
refactor/s5-shared-precision-kernels
Jun 4, 2026
Merged

refactor(s5): share GGM/Mixed precision kernels as cholesky_helpers free functions#140
MaartenMarsman merged 1 commit into
mainfrom
refactor/s5-shared-precision-kernels

Conversation

@MaartenMarsman

@MaartenMarsman MaartenMarsman commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

S5 from the class-design review, in the free-function form the review-critique settled on (not a composed GaussianPrecisionBlock).

What

The rank-2/rank-1 determinant-lemma log-ratio and the Roverato proposal-constants extraction were near-identical between GGMModel and MixedMRFModel, differing only in where the current precision entries come from: GGM stores K directly; Mixed stores Kyy = -2 * pairwise_effects_continuous_. This is the cross-model divergence the review flagged (det-lemma block appearing 4×, and the drift-guard that was added to one model and not the other).

Promotes the shared math to stateless free functions in cholesky_helpers.h:

  • log_det_ratio_edge_kernel / log_det_ratio_diag_kernel
  • precision_proposal_constants (returns the 6 reparameterization constants)

Each takes the covariance (and Cholesky), the indices, and the resolved precision scalars — GGM passes precision_matrix_ entries, Mixed passes -2 * pairwise. The six member functions (GGM::{log_det_ratio_edge,log_det_ratio_diag,get_constants}, Mixed::{log_det_ratio_yy_edge,log_det_ratio_yy_diag,get_precision_constants}) become thin adapters.

Per the critique, these are free functions, not a composed block: only these leaf kernels are genuinely identical; the "owns Σ" framing leaks (Mixed fuses the Kyy update with the OMRF marginal recomputation).

The -2/ boundary accessor (the other half of the simpler-S5 plan) is not in scope here; this PR is the cross-model kernel dedup.

Why this is safe

The two models' originals were already identical up to int-vs-double literals (1 vs 1.0, 2 vs 2.0) that don't change the double result, and the precision-source difference now passed in as scalars (-2.0 * pairwise(i,j) is the same double as Mixed computed inline).

cholesky_helpers.h now includes explog_macros.h so the kernels use the same MY_LOG/MY_EXP as the originals — required for bitwise fidelity (these can be the OpenLibM __ieee754_* variants, not std::).

Verified bitwise across every path that hits these kernels: adaptive-Metropolis GGM (no-edge δ=0/1 + edge-sel), NUTS GGM (+ mass adaptation), and adaptive-Metropolis mixed-MRF (edge-sel + no-edge, δ=0/1). identical(main, branch) == TRUE for all three. Harnesses: dev/bitwise_ggm_mh_primitive.R, dev/bitwise_ggm_nuts.R, dev/bitwise_mixed_logdet_ratio.R.

…ree functions

The rank-2/rank-1 determinant-lemma log-ratio and the Roverato proposal-constants
extraction were near-identical between GGMModel and MixedMRFModel, differing only
in where the current precision entries come from (GGM stores K directly; Mixed
stores Kyy = -2 * pairwise_effects_continuous_). This was the cross-model
divergence the class-design review flagged (det-lemma block appearing 4x).

Promote the shared math to stateless free functions in cholesky_helpers.h:
  - log_det_ratio_edge_kernel / log_det_ratio_diag_kernel
  - precision_proposal_constants  (returns the 6 reparameterization constants)
Each takes the covariance (and Cholesky), indices, and the resolved precision
scalars; GGM passes precision_matrix_ entries, Mixed passes -2*pairwise. Per the
review-critique decision, these are FREE FUNCTIONS, not a composed
GaussianPrecisionBlock (the 'owns Sigma' framing leaks; only these leaf kernels
are genuinely identical).

cholesky_helpers.h now includes explog_macros.h so the kernels use the same
MY_LOG/MY_EXP as the originals -- required for bitwise fidelity (these may be the
OpenLibM __ieee754_* variants, not std::).

GGMModel::{log_det_ratio_edge,log_det_ratio_diag,get_constants} and
MixedMRFModel::{log_det_ratio_yy_edge,log_det_ratio_yy_diag,get_precision_constants}
are now thin adapters.

Verified bitwise across all paths that hit these kernels: adaptive-Metropolis GGM
(no-edge delta=0/1 + edge-sel), NUTS GGM (+ mass adaptation), and adaptive-
Metropolis mixed-MRF (edge-sel + no-edge, delta=0/1) raw draws are digest-
identical pre/post. identical(main, branch) == TRUE for all three harnesses.
@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.64286% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.69%. Comparing base (9e21f06) to head (786d85b).

Files with missing lines Patch % Lines
src/models/mixed/mixed_mrf_metropolis.cpp 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
- Coverage   87.74%   87.69%   -0.06%     
==========================================
  Files          87       87              
  Lines       12860    12855       -5     
==========================================
- Hits        11284    11273      -11     
- Misses       1576     1582       +6     

☔ View full report in Codecov by Harness.
📢 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 merged commit e85bd6c into main Jun 4, 2026
9 of 10 checks passed
@MaartenMarsman MaartenMarsman deleted the refactor/s5-shared-precision-kernels branch June 4, 2026 19:15
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