Skip to content

Report the Pareto k diagnostic for sis() and tis() - #411

Open
avehtari wants to merge 2 commits into
masterfrom
is-pareto-k-diagnostic
Open

avehtari wants to merge 2 commits into
masterfrom
is-pareto-k-diagnostic

Conversation

@avehtari

Copy link
Copy Markdown
Member

Fixes #227.

This PR was assisted by Claude

Summary

sis() and tis() set pareto_k to 0 for every observation, so pareto_k_values() reported the best possible diagnostic value, although loo had not computed any diagnostic. psis() reports Inf when the tail has fewer than 5 draws, so for few draws the three methods disagreed (Inf for psis(), 0 for the other two). With many draws they disagreed too: psis() returned the estimated k, and the other two still returned 0.

The Pareto k diagnostic depends only on the log ratios, because psis() estimates k before it smooths the tail. sis() and tis() now compute the same k as psis() from the same log ratios. The weights of all three methods do not change.

Change

  • A new internal function pareto_k_i() in R/psis.R fits the generalized Pareto distribution to the same tail as do_psis_i(), with the same cutoff and the same posterior::gpdfit() call, and skips the smoothing. It returns Inf in the cases where do_psis_i() does: a tail shorter than 5 draws, a constant tail, or a failed fit.
  • do_sis_i() and do_tis_i() use pareto_k_i(). tis() computes k from the log ratios before truncation.
  • do_importance_sampling() gives the "Not enough tail samples" warning for all three methods. Before, only psis() gave it.
  • sis() and tis() now give the high Pareto k warning when k is above the threshold, as psis() does.
  • The documentation of pareto_k and tail_len for sis() and tis() describes the diagnostic.
  • print() of a sis_loo or tis_loo object wrote "using sis_loo ." with a space before the period. I removed the space.

For the example in #227 (3 iterations of 2 chains), all three methods now return Inf for all 32 observations and give the same warnings. With 500 and 1000 draws of example_loglik_array(), the three methods return identical k values.

sis() and tis() now fit the generalized Pareto distribution once per observation, as psis() does, so they take a little more time.

Tests

tests/testthat/test_tisis.R:

  • New test psis, sis and tis report the same Pareto k diagnostic: identical k for example_loglik_array(). An observation with t-distributed log ratios (1 degree of freedom) gives k > 0.7 and the high-k warning for each method.
  • New test psis, sis and tis report Inf for too few draws: the Inconsistent Pareto k-values for SIS? #227 example gives Inf for all observations and both warnings for each method.
  • The existing value tests call sis() and tis() with 16 and 10 draws. They expected no warning; they now expect the tail-length warning, the same one psis() gives.
  • The print snapshot has the corrected spacing.

With the master R files, the new and changed tests give 16 failures. With this PR, devtools::test() with NOT_CRAN=true passes all 1283 tests and gives no warnings (R 4.6.1, testthat 3.3.2, Linux x86_64).

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.91304% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.33%. Comparing base (fd910fe) to head (503361b).

Files with missing lines Patch % Lines
R/psis.R 62.50% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   92.48%   92.33%   -0.15%     
==========================================
  Files          31       31              
  Lines        3139     3157      +18     
==========================================
+ Hits         2903     2915      +12     
- Misses        236      242       +6     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 503361b is merged into master:

  • ✔️loo_function: 1.81s -> 1.82s [-0.21%, +0.52%]
  • ✔️loo_matrix: 1.58s -> 1.58s [-0.31%, +0.49%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent Pareto k-values for SIS?

2 participants