refactor(ggm): Tier-1 mechanical dedups (cache-invalidation mutator + delegating ctor)#138
Merged
Merged
Conversation
The MH within-K edge/diag tuning sweep and the two edge-selection accept branches each flipped constraint_dirty_ = true; theta_valid_ = false; inline -- three copies of the same 'precision/graph changed, NUTS caches are stale' rule. Extract invalidate_gradient_cache() and call it at all three sites. set_determinant_tilt is intentionally left as-is: it only sets constraint_dirty_ (gradient-engine rebuild) and must NOT also clear theta_valid_, so it does not route through the mutator. Behavior-preserving: identical flag writes. Verified bitwise -- adaptive- Metropolis GGM raw draws (incl. the edge-selection path) digest-identical.
…tructor The two GGMModel constructors shared ~18 lines of identical member initialization (priors, precision/Cholesky/covariance identity warm-start, edge indicators, proposal SDs, sparse-graph detection, MLE warm-start). The raw-data ctor differs only in deriving n (n-1 effective df) and S = X'X from the observations and in retaining observations_ for imputation. Delegate raw-data -> suf-stat; the only raw-specific work (storing observations_ when na_impute) moves to the body. initialize_precision_from_mle reads suf_stat_/ n_/edge_indicators_, never observations_, so running it in the delegated ctor before the body sets observations_ is safe. has_missing_/missing_index_ are set by a separate setter, not either ctor, so they are unaffected. Verified bitwise: na_action=impute GGM fits (edge-sel + no-edge, exercising the raw-data ctor and the in-chain imputation RNG) plus a complete-data fit are digest-identical pre/post.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #138 +/- ##
=======================================
Coverage 87.68% 87.68%
=======================================
Files 87 87
Lines 12882 12867 -15
=======================================
- Hits 11296 11283 -13
+ Misses 1586 1584 -2 ☔ 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.
Two behavior-preserving GGM Tier-1 dedups from the class-design review, each a separate commit.
1. Gradient-cache invalidation behind a named mutator
The MH proposal-SD tuning sweep and the two edge-selection accept branches each flipped
constraint_dirty_ = true; theta_valid_ = false;inline — three copies of the same "precision/graph changed, NUTS caches are stale" rule. Extractedinvalidate_gradient_cache()and called it at all three sites.set_determinant_tiltis intentionally left inline: it only setsconstraint_dirty_(gradient-engine rebuild) and must not cleartheta_valid_, so it does not route through the mutator.2. Delegate the raw-data constructor to the suf-stat constructor
The two constructors shared ~18 lines of identical member init (priors, identity warm-start of precision/Cholesky/covariance, edge indicators, proposal SDs, sparse-graph detection, MLE warm-start). The raw-data ctor differs only in deriving
n(n-1 effective df) andS = X'Xfrom the observations, and in retainingobservations_for imputation. It now delegates; the only raw-specific work (observations_ = observationswhenna_impute) moves to the body.Safe because:
initialize_precision_from_mle()readssuf_stat_/n_/edge_indicators_, neverobservations_, so running it in the delegated ctor before the body setsobservations_is fine; andhas_missing_/missing_index_are set by a separate setter, not either ctor.Verification
Both commits verified bitwise-identical:
dev/bitwise_ggm_mh_primitive.R.na_action="impute"fits (edge-sel + no-edge, exercising the raw-data ctor and the in-chain imputation RNG) plus a complete-data fit digest-identical. Harnessdev/bitwise_ggm_ctor.R.identical(main, branch) == TRUEin both.