use new k threshold

[avehtari]

Fixes #234

Ping @n-kall

[jgabry]

I haven't had a chance to go through this in detail, but looks like it causes several tests to fail. Also I wonder if there will be any issue with reverse dependencies, but we can check that later.

[jgabry]

@avehtari CRAN wants a new release of loo before February 20th to fix some minor documentation issue that doesn't affect anything and is only flagged in the newest version of R. We can include this if we fix the tests and check reverse dependencies. I can check the reverse dependencies once everything is passing.

[jgabry]

These changes look good, except for a few issues I commented on.

[avehtari]

Yesterday, I started working on fixing the PR, and I'm sorry for being so sloppy the first time and wasting your time. As there were still some checks failing, I didn't push the changes that I had made so far. Thanks for the additional comments. l'll keep working on this today.

[avehtari]

• more updates on the code and docs, so a new review would be good
• I used attribute to pass the threshold along the pareto k table for printing, is this ok?
• moment matching test results have changed due to changed threshold, did not yet update
• loo_subampling has some tests errors I haven't yet been able to solve, but I'll keep working on those, but I expect that the tests need to be fixed

[n-kall]

I went through the doc changes, and made a few minor suggestions

[jgabry]

Changes look good. I made a bunch of tiny suggestions.

[jgabry]

I haven't tested all the changes to the plotting code, but they look ok

[avehtari]

Tests are passing on my computer. I still need to write more documentation.

[codecov-commenter]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (dab6e58) 93.00% compared to head (258adc8) 92.41%.

❗ Current head 258adc8 differs from pull request most recent head 372cb92. Consider uploading reports for the commit 372cb92 to get more accurate results

Files	Patch %	Lines
R/diagnostics.R	91.89%	3 Missing ⚠️
R/loo.R	71.42%	2 Missing ⚠️
R/psis.R	71.42%	2 Missing ⚠️
R/effective_sample_sizes.R	50.00%	1 Missing ⚠️
R/print.R	96.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   93.00%   92.41%   -0.59%     
==========================================
  Files          30       30              
  Lines        2788     2823      +35     
==========================================
+ Hits         2593     2609      +16     
- Misses        195      214      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[avehtari]

Improved docs, fixed a few more things in code

[jgabry]

Great, looks like tests are passing on GitHub too, it's just R cmd check is failing due to a documentation issue: https://github.com/stan-dev/loo/actions/runs/7686962192/job/20946444630?pr=235#step:6:167. It might actually be that the doc just needs to be regenerated to match the code.

[avehtari]

All the vignettes have been now updated. I couldn't run loo2-non-factorized vignette as installation of spdep package failed (only data used from that package), but there shouldn't be any issues with that vignette.

[avehtari]

Should be complete now

[n-kall]

Looked through the vignette changes. Looks good, only found one thing

[jgabry]

There's one problem with the changes to the mixis vignette that will cause an error (see review comment) but otherwise these vignette updates look ok to me

[avehtari]

Here is a proposal for a longer news item

There are two major changes in the new loo release

1. Use of new sample size specific diagnostic threshold for Pareto k. Pre-2022 PSIS paper recommended diagnostic thresholds as k<0.5 "good", 0.5 <= k <0.7 "ok", 0.7 <= k < 1 "bad", k>=1 "very bad". 2022 PSIS paper recommends k < min(1 - 1/log10(S), 0.7) "good", min(1 - 1/log10(S), 0.7) <= k < 1 "bad", k > 1 "very bad", where S is the sample size. There is one diagnostic threshold less, and the most important threshold now depends on the sample size S. With sample sizes 100, 320, 1000, 2200, 10000 the sample size specific part 1 - 1/log10(S) correspond to thresholds 0.5, 0.6, 0.67, 0.7, 0.75. Even if the sample size grows, the bias in PSIS estimate dominates if 0.7 <= k < 1, and thus the diagnostic threshold for good is capped at 0.7 (if k>1, mean does not exist and bias is not valid measure). The new recommended thresholds are based on more careful bias-variance analysis of PSIS based on truncated Pareto sums theory. For those who use the Stan default 4000 posterior draws the 0.7 threshold will be the same, but there will be less warnings as there will be no diagnostic message for 0.5 < k 0.7. Those who use small sample sizes, may see diagnostic messages with threshold less than 0.7, and they can simply increase the sample size to about 2200 to get the threshold 0.7.

2. There are no more warnings if r_eff argument is not provided, and the default is now 1. The summary print output showing MCSE and ESSs shows diagnostic information on the range of r_eff. The change was made to reduce unnecessary warnings. Use of r_eff does not change the expected value of elpd_loo, p_loo, and Pareto k', and is needed only to estimate MCSE and ESSs. Thus it is better to show the diagnostic information about the used r_eff only when MCSE and ESS values are shown.

[jgabry]

Here is a proposal for a longer news item

These look good. I made a few tiny edits and added them to the NEWS file. I will update the rest of the NEWS file to reflect some other changes we made in other PRs before releasing.

[jgabry]

I can't think of anything else we need to do before merging this. @avehtari or @n-kall, do you have anything else or should I go ahead and merge? (I will still wait about a week to do the release.)

[n-kall]

do you have anything else or should I go ahead and merge?

Nothing that I can think of from my side. I think the next related thing will be to update the documentation for posterior::pareto_khat (and others) to match these updates, but I don't see any necessary changes needed in loo for that

[jgabry]

Nothing that I can think of from my side.

Ok cool. And thanks for helping to review this.

I think the next related thing will be to update the documentation for posterior::pareto_khat (and others) to match these updates, but I don't see any necessary changes needed in loo for that

Sounds good, thanks.

[jgabry]

@avehtari If I submit a bayesplot release with Teemu's fix for his recent PR, is that sufficient to fix the issue with the pit plots in the loo vignette or does the vignettes need additional editing after that? I think I will go ahead and submit that patch release of bayesplot today or tomorrow (it only contains that one bug fix).

[avehtari]

Teemu's bayesplot PR and fix for discrete data PIT affects ppc_pit_ecdf*, but not ppc_loo_pit_qq. Teemu's rsantools PR stan-dev/rstantools#121 fixes loo_pit and thus fixes ppc_loo_pit_qq used in loo2-example.Rmd

[jgabry]

Oh ok thanks, that makes sense.

[jgabry]

Merging now!