Report the Monte Carlo SE of the mean from the draws themselves - #116
Conversation
With the cmdstanr engine `fit$summary$se_mean` was the posterior SD over the square root of `posterior::ess_bulk()`. Bulk ESS is computed on rank-normalized draws, so it measures autocorrelation of the ranks, not of the draws whose mean is being reported, and the two differ whenever the draws are skewed. For a lognormal AR(1) chain with autocorrelation 0.75, as an exponentiated effect such as a risk ratio behaves, the old value is 13.8% too large, and more draws do not remove it. `se_mean` now comes from `posterior::mcse_mean()`, which uses the ESS of the draws themselves. The bulk and tail ESS columns are unchanged. Nothing in the package prints or uses this column; it reaches a user who reads `fit$summary` directly. The rstan engine is unaffected. Checks: the cmdstanr end-to-end test now asserts `se_mean` against `posterior::mcse_mean()` for `mu_index` and `rr_index`; it fails against main and passes here. Pure-R suite: 0 failures, 0 errors, 91 skips, 2,829 passes. Version stays 0.1.0.9000.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: choxos/mlumr/.coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe CmdStanR summary now calculates ChangesCmdStanR se_mean calculation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The CmdStanR summary change is documented and covered by the targeted end-to-end assertions. No actionable merge risk remains after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
What was wrong
With the cmdstanr engine,
fit$summary$se_meanwas computed assd / sqrt(posterior::ess_bulk()). Bulk ESS is computed on rank-normalized draws, so it describes the autocorrelation of the ranks rather than of the draws whose mean is reported. The two differ whenever the draws are skewed, as an exponentiated effect such as a risk or rate ratio is.For four stationary chains of
exp(Z), withZan AR(1) process with autocorrelation 0.75, the exact Monte Carlo SE of the mean and the old column compare as follows:se_meanposterior::mcse_mean()The old value is about 13.8% too large in the limit, and more draws do not remove it.
What changes
se_meancomes fromposterior::mcse_mean(), the mean-specific MCSE. The bulk and tail ESS columns are unchanged.fit$summarydirectly. The rstan engine is unaffected.Checks
test-engine.Rnow assertsse_meanagainstposterior::mcse_mean()formu_indexandrr_index. It fails againstmainand passes here.Independent of #115; either can merge first. Version stays
0.1.0.9000.Summary by CodeRabbit