Clarify print output to specify posterior draws and log-likelihood terms#328
Clarify print output to specify posterior draws and log-likelihood terms#328ishaan-arora-1 wants to merge 2 commits intostan-dev:masterfrom
Conversation
The previous print output "Computed from 4000 by 262 log-likelihood matrix" was ambiguous about which dimension was draws and which was observations. This made it easy for users to miss cases where they accidentally misspecified the log-likelihood (e.g. placing all observations in one multivariate normal). The new output explicitly labels both dimensions: "Computed from 4000 posterior draws and 262 log-likelihood terms." Updated all print_dims methods (psis_loo, importance_sampling, importance_sampling_loo, waic, psis_loo_ss, elpd_generic), along with corresponding tests, snapshots, and vignette output. Fixes stan-dev#198
|
Hi @ishaan-arora-1, can you clarify why did you close this PR? Based on a quick look, it seems to be useful |
Hey, i just wanted to review, why the tests weren't passing before reopening it. I am happy that this pr sounds useful. i'll try to get the tests working in a few hours. |
|
In some of our repos there have been some CI tests failing due to Windows problems. It may take some days before your recent PRs are reviewed. Thanks for submitting them |
|
Glancing at the failed logs, I think tests need to be upated to reflect the new documentation. |
|
do you maybe want a hand in that? just lemme know, ill be happy to contribute.
|
|
Yes, do you want to take a stab at it? The tests are expecting "subsampled log-likelihood values" but the new message is "subsampled log-likelihood terms", so the test case should reflect that. |
sure, would love to :) |
The print_dims.psis_loo_ss method now says "log-likelihood terms" instead of "log-likelihood values", so update the test expectations to match.
|
Fixed! The tests in Ran the full test suite locally, 1160 pass, 0 fail, 0 warnings (2 skips are the usual M1 Mac platform skips). Any feedback @avehtari @VisruthSK |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #328 +/- ##
==========================================
- Coverage 92.78% 92.77% -0.01%
==========================================
Files 31 31
Lines 2992 2991 -1
==========================================
- Hits 2776 2775 -1
Misses 216 216 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Lgtm. If Aki likes the language I think this is good to merge. The only thing I can think of is maybe the tests that you just had to fix should be snapshot tests instead, but that change needn't be made in this PR. |
sounds good, i can do that in another pr. |
4 participants
Hey! This addresses #198, where the current print output like
was flagged as ambiguous — it's not immediately clear which dimension is the number of posterior draws and which is the number of observations/log-likelihood terms. As mentioned in the issue, ParetoSmooth.jl already uses a more explicit format, and this kind of ambiguity can hide bugs (e.g. a user accidentally placing all observations in one multivariate normal).
Changes
The new output format is:
This is close to what @avehtari proposed in the issue comments (
"Computed from 4000 posterior draws by 262 log-likelihood terms matrix"), but I dropped "matrix" since the user doesn't really need to know it's a matrix — they care about what the numbers mean.For log-weight objects (raw
psis/tis/sis):For subsampled loo:
Updated methods
print_dims.psis_looprint_dims.importance_samplingprint_dims.importance_sampling_looprint_dims.waicprint_dims.psis_loo_ssprint_dims.elpd_genericAlso updated
test_print_plot.Randtest_loo_subsampling_cases.Rpsis.md,tisis.md, andloo_moment_matching.mdloo2-with-rstan.Rmdandloo2-large-data.RmdNEWS.mdentryFixes #198