Rely on posterior for pareto smooth tails

[VisruthSK]

Rely on posterior for pareto smoothening tails.

[VisruthSK]

Make sure to bump version in DESCRIPTION when posterior gets updated

[VisruthSK]

Updated snaps just to see if tests pass, but there are a lot of (repeated) warnings in the new version.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.76%. Comparing base (df56db4) to head (1593dea).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   92.73%   92.76%   +0.02%     
==========================================
  Files          31       31              
  Lines        3057     2998      -59     
==========================================
- Hits         2835     2781      -54     
+ Misses        222      217       -5     

☔ View full report in Codecov by Sentry.
📢 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.

[avehtari]

Unfortunately posterior has many warnings and the progress on having better control of the warnings with warning levels has been slow stan-dev/posterior#309

[jgabry]

Unfortunately posterior has many warnings and the progress on having better control of the warnings with warning levels has been slow stan-dev/posterior#309

Yeah and warnings add some overhead (even if suppressed), so I expect this to be a bit slower than the current implementation in the case that there are many warnings. I wish there was just a quiet=TRUE argument to posterior::ps_tail() that we could set when calling it from loo since we already have a better warning in loo for this. But it's probably not so much slower, although answering questions like that is what prompted my suggestion in #348.

[avehtari]

Just in case if you had not noticed posterior::pareto_smooth() and posterior::ps_tail() were fixed about two weeks ago stan-dev/posterior#442

[jgabry]

No I missed that PR, I hadn't seen that

[jgabry]

@avehtari it would also be good if posterior::ps_tail() had an option to accept already sorted input. Right now we have to sort in loo to figure out if there are enough tail samples but then ps_tail() sorts again internally. Previously since we were already sorting in loo we could just reuse the sorted vector, but now we have to do two sorts, which adds overhead. I'm not sure how much, but #352 can maybe help figure that out.

[jgabry]

I think this looks good now, the only question I have is whether we care about the extra sort that we need to do (see previous comment). I don't think there's a way to avoid it unless posterior::ps_tail is updated.

[VisruthSK]

Seems like its probably worth opening a PR to modify ps_tail then to add two flags, sorted and silent?

[avehtari]

Before adding silent flag to any function, I think we should decide whether we would in general prefer to use per function flags or global package options. See also stan-dev/posterior#309, stan-dev/cmdstanr#1175, and #353

[avehtari]

Performance

Using the 4000x1359 log_lik_matrix from #340 (comment), current posterior::ps_tail() is 12% slower than current loo:::do_psis_i

> tic();q=replicate(10,apply(log_lik_matrix, 2, \(x) loo:::do_psis_i(x, 190)));toc()
6.654 sec elapsed
> tic();q=replicate(10,apply(log_lik_matrix, 2, \(x) ps_tail(x, 190, tail="right", are_log_weights=TRUE)));toc()
7.443 sec elapsed

I did not test with this PR. As that log_lik_matrix is considered to be added for tests in that PR, maybe the same can be used for performance testing.

(I'll add later comments about desired behavior which helps to clarify which warnings we want to see)

[VisruthSK]

Thanks! I forgot about that example from that PR, good idea to add it to #352.

[avehtari]

• I rerun the speed experiment and now posterior::ps_tail() has the same speed (without any modifications)
• In psis-loo, constant tail is problem only for computing Pareto-k diagnostic. Only problematic constant tail case would be if all log liks are -Inf or Inf, but that is very unlikely and would cause other errors. If log_liks are finite then constan tail means many clearly non-zero weights which is fine for importance sampling. Thus we don't need to give a warning about constant tail, we just return the tail as is, and Pareto-k as NA. It's a later problem how to handle that NA in diagnostic messages and print.
• In posterior package, the ndraws_tail < 5 warning in ps_tail() is never happening as pareto_smooth() sets ndraws_tail to be at least 5. I think that the ndraws_tail < 5 warning in ps_tail() could be just removed.
• Years ago I did use r_eff_i to compute tail_len_i, but I would not do that anymore. This would simplify the code as we would need to compute tail_len only once. We can get tail_len < 5 only if the size of the posterior sample S < 21, which is very unlikely. With just one tail_len, if S < 21, we could just return all log_liks as they are and do_psis_i doesn't need to check for this.

[jgabry]

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

• ❗🐌loo_function: 1.99s -> 2.16s [+7.37%, +9.46%]
• ❗🐌loo_matrix: 1.54s -> 1.71s [+10.41%, +11.46%]
  Further explanation regarding interpretation and methodology can be found in the documentation.

So here we're seeing a non-trivial slowdown. Although we should probably increase the number of repetitions before inferring too much.

[VisruthSK]

Touchstone now is using 60 reps per branch. Will see if there's still a slowdown.

[github-actions]

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

• ❗🐌loo_function: 2.04s -> 2.19s [+7.21%, +7.8%]
• ❗🐌loo_matrix: 1.63s -> 1.8s [+10.28%, +11.43%]
  Further explanation regarding interpretation and methodology can be found in the documentation.

[VisruthSK]

Seems like posterior's implementation is still slow. Should we get these fixes into posterior before merging this? Or is a ~10% slowdown okay, and we'll get a speedup once posterior is updated and ps_tail() is faster.

It's a bit of a pain but I'm reluctantly in favor of waiting.

[avehtari]

~10% slowdown matches speed comparison on my laptop. posterior package has at least more conditionals as the function supports more options. The question is how much we want to specialize e.g. by making a separate function that is only for log ratios (no check whether non-log vs log, and no check for which tail).

[VisruthSK]

Separate function makes sense to me. Are there any real downsides to that approach?

[avehtari]

Downside is having repeated code and if behavior of one function is changed, do we remember to change the behavior of the other function. This has been especially problem when pareto related code has been replicated in different packages. It should be more likely that we remember fix two versions if they are in the same package.