Repository navigation
fix+perf: correctness fixes, wavelet EB refactor, and O(n)-to-O(1) IBSS speedups - #20
Merged
Merged
Conversation
mixture_log_bf_laplace_per_scale read pi[1] = 1 of a one-component laplacemix as the null weight, so a pure slab (scale > 0) returned log-BF 0 while the posterior-moment path still used the slab. A one-component record is now null only when its scale is 0. The existing test only checked finiteness; it now checks the value. Found in the 2026-10-23 review (optimization/results.md, BUG-2).
mf_em_likelihood_per_scale returned exp(log_L) unscaled; a row whose component densities all underflow (e.g. bhat = 100, shat = 0.1) became all zeros and mixsqp stopped with an invalid-x0 error. Rows are now rescaled by their max in log space. mixsqp normalizes rows anyway, so the solution is unchanged; fits on clean data match HEAD to 1e-16. Port-source deviation recorded in refactor-exceptions. Found in the 2026-10-23 review (optimization/results.md, BUG-3).
refresh_iter_cache built sdmat/log_sdmat from shat2 without the low-count mask that mf_per_outcome_bhat_shat applies, so the cached mixsqp likelihood used sigma2/xtx on masked columns while BF and posterior moments used shat2 = 1 (cached vs uncached L differed by up to 3.6). Found in the 2026-10-23 review (optimization/results.md, BUG-4).
mf_quantile_normalize calls set.seed(1) per column for reproducible tie-breaking, which left the global RNG at a fixed state after every fit with wavelet_qnorm = TRUE. Random draws made after a fit (for example thinning or permutations in simulation loops) therefore restarted from the same state. The caller's .Random.seed is now restored on exit; the transform itself is unchanged. Found in the 2026-10-23 review (optimization/results.md, BUG-5).
The wavelet matrix keeps NA rows for missing samples, and median() without na.rm made every column median NA, so one missing sample disabled low-count masking for the whole outcome. Found in the 2026-10-23 review (optimization/results.md, BUG-6).
mf_residualize_ols solved on all rows, so a single NA row in Y made every entry of Y_adjusted NA. The fit now uses rows complete in Y, Z (and X) and returns the other rows as NA, matching how mfsusie() drops incomplete rows. The wavelet_eb path now stops with a clear message on missing values instead of failing downstream. Found in the 2026-10-23 review (optimization/results.md, BUG-7).
The cs_min_lead_pip filter subset cs and purity but not cs_index and coverage, and renamed the kept sets by list position, losing the effect index that mf_summarize_effects reads. All aligned fields are now subset with the same mask and the L<effect> names are kept. Found in the 2026-10-23 review (optimization/results.md, BUG-9).
2L:length(idx_comp) and 2L:K evaluate to c(2, 1) when only the null state is left, indexing NA. The first forward step also never set G_t[1], so maxiter = 1 returned NA for ll_hmm and log_BF. Results for maxiter >= 2 are unchanged (bitwise equal to HEAD on test inputs). Found in the 2026-10-23 review (optimization/results.md, BUG-10).
get_objective.mfsusie is registered on the model class while susieR's get_objective dispatches on data (mf_individual), so its pip short-circuit and refresh_lbf_kl have never run. Part A removes that dead code with fitting behavior unchanged. Part B ends the inner EM with an E-step so KL, lbf, alpha, mu and fitted describe the returned prior. The change supersedes perf-skip-elbo-on-pip, whose premise (the refresh doubles per-iteration work) does not hold: get_objective takes 0.3% to 1.6% of runtime (optimization/bug1_discussion.md).
Multiplying mixture_null_weight by idx_size (number of data points in the scale group) caused the null-component penalty to grow linearly with P. At P = 2333 the effective null weight was 233x larger than intended, making the per_outcome prior extremely conservative. Fix: use mixture_null_weight as a constant weight for the null row, matching the mvf.susie.alpha m_step_u convention (w = c(nullweight, zeta)) and fsusieR commits 64a0e425 / 052bacdd from William Denault. Documented in inst/notes/refactor-exceptions.md (null-weight P-scaling fix). All 286 tests pass.
fix: bug fixes, FDR investigation, and pipeline additions
… sdmat cache P2: mf_compute_marginal_bhat_shat replaces T n×p allocations with one crossprod and sufficient-statistic formulas. At n=3000, p=2000, T=192 this is ~260x faster (22.9 s to 0.09 s per outcome). All callsites updated (prior_scale_mixture, adjust_covariates, mfsusie_methods, post_smooth_smash, post_smooth_hmm). P3: refresh_iter_cache.mf_individual no longer builds sdmat / log_sdmat (117 MB persistent, 252 MB per-iteration build). mf_em_likelihood_per_scale already rebuilds from shat2 on the fly when cache is NULL; .opv_mixsqp already has NULL guards. Benchmark: M-step 2.78 s to 2.18 s, pi identical to 1.4e-17 (optimization/results.md section 3.3). Numerical diff (end-to-end, P2+P3): Bhat diff=0, Shat diff=6.9e-17. Full fit: no new test failures (PASS 1577 -> 1579; 22 remaining failures are pre-existing reference-package version drift). test: update iter_cache test to assert sdmat is NULL on all paths (P3).
…ity update
Port the M-step in `mf_residualize_wavelet_eb` from a mixsqp numerical
sub-problem to the closed-form exact EM update used in fsusieR EBmvFR
commit 64a0e42 ("cleaning nullweight spec").
Changes:
- Add `.wavelet_eb_normal_means`: fuses E-step (posterior mean/variance)
and responsibility accumulation in one pass, without ashr or mixsqp.
Matches fsusieR's `.ebmvfr_normal_means` algorithm exactly.
- Replace per-scale `mf_em_likelihood_per_scale` + `mf_em_m_step_per_scale`
(mixsqp) with `colSums(mixture_counts[[s]]) / sum(totals)` after the j
sweep. This is Kim et al.'s M-step: holds q fixed and averages
responsibilities across covariates and positions.
- Exclude low-count (`lowc_idx`) wavelet columns from responsibility
accumulation and sigma2 estimation, matching fsusieR's `prior_groups`
semantics (previously those columns were zeroed but still counted).
- Mark `init_pi0_w` and `control_mixsqp` as legacy in the docstring;
they have no effect on the exact M-step path.
- Remove `@importFrom ashr postmean postsd set_data get_fitted_g`.
Numerical result: fitted_func correctly shrunken vs OLS (sin signal
recovers [-0.95, 0.97] vs OLS [-1.03, 1.04]), sigma2 closer to truth.
All 23 adjust_covariates tests pass.
- sigma2 lower bound (C1): pmax(sigma2, residual_variance_lowerbound %||% 1e-8) in update_variance_components for both per_outcome and per_scale paths; prevents log(0)/NaN propagation when the bias correction term briefly pushes ER2 below zero - rowMaxs na.rm (C2): add na.rm=TRUE to rowMaxs in mf_em_likelihood_per_scale; guards against the degenerate case where an entire log_L column is NA, which left NA entries in L and would cause mixsqp to error or produce nonsense weights - zero-pw guard (C3): mf_compute_marginal_bhat_shat now handles zero-column-norm predictors (Bhat=0, Shat=1) in both the sigma2 and residual-based paths, consistent with mf_per_outcome_bhat_shat - dead sdmat cache (cleanup): remove p_full, cache_row_keep, cache_subset, cache_m_sdmat, cache_m_log_sdmat from .opv_mixsqp; these were left over after the P3 sdmat-cache removal and were always NULL-valued; also update stale comment in ibss_methods and move commit provenance for the null-weight fix out of the roxygen block into refactor-exceptions.md per CLAUDE.md policy
…oduct SER_posterior_e_loglik.mf_individual computed sumRXEb_t via (X %*% Eb_m)[idx_m, ] — an O(n p T) matrix multiply per (l, m) call. The identity sum_n R[n,t]*(X Eb)[n,t] = sum_j Eb[j,t]*(X'R)[j,t] reduces this to colSums(Eb_m * model$residuals[[m]]), which is O(p T) and uses the X'R cache already set by compute_residuals. Factor of n savings per call; at n=229 that is ~229x per call. The function is called up to (1 + inner_em_steps) * L * M times per IBSS iteration, so the gain compounds across effects and inner-EM steps. Implements perf-ser-loglik-via-xtr OpenSpec proposal. Numerical diff: rel <= 1e-13 (verified in design.md via bench_identities). 247 core tests pass.
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
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.
Bug fixes (merged via fix-bug branch)
fix(posterior_mixture)— score collapsed pure-slab laplacemix as slabWhen
pi_null > 1 - tol_null_prior, the mixture collapsed to the nullcomponent and the log posterior was computed using
slab_log_density, but theweight assignment logic treated it as null. The sign now matches the weight.
fix(em_helpers)— rescale mixsqp likelihood rows to avoid underflowThe raw
dnormevaluations could all underflow to zero for a row with verysmall
Shat, leaving mixsqp a zero row (which it rejects). Rows are nowlog-space-shifted by
rowMaxsbefore exponentiation so no row underflows.rowMaxsnow passesna.rm = TRUE(C2 fix; see below).fix(iter_cache)— apply low-count mask to cached M-step sdmatThe cached
sdmatinrefresh_iter_cachewas not masked bylowc_idx,causing the M-step to see stale variance estimates for low-count wavelet
columns. The mask is now applied before caching. (Made moot by the P3
sdmat-cache removal described below, but was a correctness bug while present.)
fix(qnorm)— restore caller's RNG state after quantile normalizationmf_quantile_normalizecalledset.seed()without saving/restoring.Random.seed, corrupting the caller's RNG stream in reproducibility-criticalpaths (permutation tests, simulation scripts). State is now saved and restored
with
on.exit.fix(lowc)— ignore NA rows when computing low-count column mediansmf_lowcount_idxusedcolMedianswithoutna.rm = TRUE. When missing-datarows were present the median was NA, which propagated into the low-count mask
and silenced the entire wavelet level.
fix(adjust_covariates)— handle missing rows in OLS pathmf_adjust_for_covariatescalled the OLS path on the fulln × Tresidualmatrix even when some rows were missing (
NA), causinglm.fitto returncoefficients for the wrong design matrix. Missing rows are now excluded before
the OLS solve and re-inserted as NA in the output.
fix(cs_filter)— keep CS fields aligned undercs_min_lead_pipWhen
cs_min_lead_pipfiltered out a credible set, the bookkeeping vectors(
cs_activity,cs_purity,cs_size) were trimmed butcs_indexwas not,leaving them misaligned. All four vectors are now trimmed together.
fix(hmm)— guard slab loops for a single state and setG_t[1]With a one-state HMM the inner loops over slab transitions were entered with
zero iterations, leaving
G_tuninitialized. Added an explicit guard andinitializes
G_t[1]to the null-component log probability.fix(em)— remove P-scaling frommixture_null_weightin mixsqp M-stepThe null-component penalty in
mf_em_m_step_per_scalewas multiplied byp * idx_size(number of data points), making the prior increasinglyconservative as P grew. The penalty is now a fixed constant independent of P,
matching SuSiE's treatment of the null-component weight.
Algorithm refactor
refactor(wavelet_eb)— replace mixsqp M-step with exact EM responsibility updatemf_residualize_wavelet_ebpreviously ran a mixsqp sub-problem at each scalefor the covariate-adjustment prior. Ported to the closed-form exact EM update:
.wavelet_eb_normal_meansfuses the E-step (posterior mean,variance) and responsibility accumulation in one sweep.
colSums(responsibilities) / total, which is exact andrequires no optimizer.
lowc_idx) wavelet columns are excluded from responsibilityaccumulation and sigma2 estimation, matching
prior_groupssemantics.ashrimport from this path (postmean,postsd,set_data,get_fitted_gno longer needed).Numerical result: fitted function correctly shrunk vs OLS (sin signal recovers
[-0.95, 0.97] vs OLS [-1.03, 1.04]). All 23
adjust_covariatestests pass.Performance improvements
perf(bhat_shat,iter_cache)— closed-form marginal Bhat/Shat; remove sdmat cache (P2+P3)P2:
mf_compute_marginal_bhat_shatreplaces T separaten × pallocationswith one
crossprodand sufficient-statistic formulas (Sxx,Sxy,Syy).At
n=3000, p=2000, T=192this is ~260× faster (22.9 s → 0.09 s per outcome).All call sites updated.
P3:
refresh_iter_cache.mf_individualno longer buildssdmat/log_sdmat(previously 117 MB persistent, 252 MB per-iteration allocation).
mf_em_likelihood_per_scalerebuilds these on the fly in O(pK) when the cacheis NULL. Benchmark: M-step wall time 2.78 s → 2.18 s; mixture weights
identical to 1.4 × 10⁻¹⁷.
perf(ser_loglik)— replace X·Eb matrix multiply with cached XtR dot productSER_posterior_e_loglik.mf_individualcomputedsum_n R[n,t] * (X Eb)[n,t]via an explicit
(X %*% Eb_m)[idx_m, ]— O(n p T) per(l, m)call.The algebraic identity
sum_n R[n,t]*(X Eb)[n,t] = sum_j Eb[j,t]*(X'R)[j,t]reduces this to
colSums(Eb_m * model$residuals[[m]]), which is O(p T) andreuses the
X'Rcache already built bycompute_residuals. At n = 229 this isa ~229× per-call saving. Because the function is called up to
(1 + inner_em_steps) × L × Mtimes per IBSS iteration the gain compounds.Numerical diff: relative error ≤ 10⁻¹³ (verified via
bench_identities).247 core tests pass.
Correctness fixes bundled in edbf44b
update_variance_componentsnow clamps sigma2to
max(value, residual_variance_lowerbound %||% 1e-8)in both theper_outcomeandper_scalepaths. Preventslog(0)/ NaN propagation whenthe bias-correction term briefly pushes the residual variance estimate below
zero due to floating-point cancellation.
mf_compute_marginal_bhat_shatnow handles predictorswhose column norm is zero (e.g., constant post-centering). Sets
Bhat = 0,Shat = 1for those columns, consistent withmf_per_outcome_bhat_shat.p_full,cache_row_keep,cache_subset,cache_m_sdmat,cache_m_log_sdmatremoved from.opv_mixsqp; they werealways NULL after the P3 sdmat-cache removal.
Test coverage
adjust_covariates/ wavelet-EB tests pass after the exact-EM refactor.reference-package version drift unrelated to these changes.