improved E_loo Pareto-k diagnostics
Fixes #236
- Uses the recommended h-specific Pareto-k approach
- Uses type specific h when computing Pareto-k
- Documents the used h-specific Pareto-k approach in the doc
- Uses
posterior::pareto_khat, but this can be copied tolooif we want to avoid the dependency
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
68ee019) 92.44% compared to head (1663687) 92.78%. Report is 2 commits behind head on master.
:exclamation: Current head 1663687 differs from pull request most recent head 69de50f. Consider uploading reports for the commit 69de50f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## master #247 +/- ##
==========================================
+ Coverage 92.44% 92.78% +0.34%
==========================================
Files 31 31
Lines 2831 2829 -2
==========================================
+ Hits 2617 2625 +8
+ Misses 214 204 -10
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This looks good, but I'm still not sure what to do about the posterior dependency. If we keep it like this then we should move it to Imports from Suggests, but I think my slight preference would be to copy anything needed over to loo, but just for right now. Then eventually we could do a bigger update where we convert to using posterior everywhere in the loo package where it makes sense (and then delete the copied code and add a strong dependency on posterior). For example, I think we can also use posterior::pareto_smooth to do the smoothing in loo::psis(), right? What do you think of that option?
- Direct copying of code from
posterioris 400 lines of code, which is a lot compared to current 200 lines of code inE_loo.R - We could use existing functions in psis.R, but as they have been designed to work on log weights, there would be need for additional code to make them work for both tails of hr which may have negative values, and to include the safety checks included in the posterior package.
Option 1. is easy as I already tested it (by keeping copying missing functions until it did work), but is 400 lines too much?
I didn’t realize it would be that much code. Thats a good reason for just adding the dependency on posterior since we probably don’t want to maintain a lot of duplicate code in different packages. But I’ll think about it a little more today before we make a final decision.
It's about 200 lines for Pareto diagnostic and smoothing which supports both non-logged and logged inputs and both tails, and about 200 lines for ess_tail and helper functions.
I guess a simple version which would use only functions already in loo would be about 60 lines of code and it would take 30 mins to write and test, but it would be less robust without checks in posterior package version.
Ok I think let's just go ahead and put posterior in Imports. Since many of our other packages already import (or will import) posterior I guess it's not such a big deal for loo to import it too. Then we can also remove some code in effective_sample_sizes.R where we copied the autocovariance function (and in the future use any code from posterior that we want). I will push a commit now that does this.
Ok everything is passing so I will merge this and then start running reverse dependency checks again
Reverse dependency checks passed except for the one package we already knew about. I submitted a PR to fix that package almost a month ago (and it was merged already), so I will go ahead and submit loo to CRAN.
so I will go ahead and submit loo to CRAN.
On CRAN now