Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improved E_loo Pareto-k diagnostics #247

Merged
merged 4 commits into from
Feb 22, 2024
Merged

improved E_loo Pareto-k diagnostics #247

merged 4 commits into from
Feb 22, 2024

Conversation

avehtari
Copy link
Collaborator

Fixes #236

  1. Uses the recommended h-specific Pareto-k approach
  2. Uses type specific h when computing Pareto-k
  3. Documents the used h-specific Pareto-k approach in the doc
  4. Uses posterior::pareto_khat, but this can be copied to loo if we want to avoid the dependency

@avehtari avehtari requested a review from jgabry February 17, 2024 12:14
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (68ee019) 92.44% compared to head (1663687) 92.78%.
Report is 2 commits behind head on master.

❗ 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     

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

@jgabry
Copy link
Member

jgabry commented Feb 21, 2024

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?

@avehtari
Copy link
Collaborator Author

  1. Direct copying of code from posterior is 400 lines of code, which is a lot compared to current 200 lines of code in E_loo.R
  2. 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?

@jgabry
Copy link
Member

jgabry commented Feb 21, 2024

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.

@avehtari
Copy link
Collaborator Author

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.

@jgabry
Copy link
Member

jgabry commented Feb 21, 2024

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.

@jgabry
Copy link
Member

jgabry commented Feb 22, 2024

Ok everything is passing so I will merge this and then start running reverse dependency checks again

@jgabry jgabry merged commit a362836 into master Feb 22, 2024
6 checks passed
@jgabry jgabry deleted the E_loo-improvements branch February 22, 2024 16:32
@jgabry
Copy link
Member

jgabry commented Feb 24, 2024

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.

@jgabry
Copy link
Member

jgabry commented Feb 25, 2024

so I will go ahead and submit loo to CRAN.

On CRAN now

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.

Update E_loo khat diagnostic
3 participants