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

Add relative efficiency function #354

Closed
wants to merge 3 commits into from
Closed

Conversation

n-kall
Copy link
Collaborator

@n-kall n-kall commented Mar 13, 2024

Summary

This would fix #280.

I am revisiting r_eff while implementing the weighted MCSE and ESS, which should take this into account (if draws are from MCMC).

I implemented relative_eff as ess_basic(x) / ndraws(x). This gives slightly lower (about 1%) results than loo::relative_eff. I guess due to the difference between loo:::rfun_ess and posterior::ess_basic

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.34%. Comparing base (9de9857) to head (34443df).

❗ Current head 34443df differs from pull request most recent head 917c2e8. Consider uploading reports for the commit 917c2e8 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #354   +/-   ##
=======================================
  Coverage   95.33%   95.34%           
=======================================
  Files          50       50           
  Lines        3855     3863    +8     
=======================================
+ Hits         3675     3683    +8     
  Misses        180      180           

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

Copy link

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

  •   :ballot_box_with_check:as_draws_array: 142ms -> 141ms [-1.28%, +0.85%]
  •   :rocket:as_draws_df: 66.5ms -> 65.4ms [-2.83%, -0.42%]
  •   :rocket:as_draws_list: 162ms -> 160ms [-2.35%, -0.24%]
  •   :rocket:as_draws_matrix: 66ms -> 64.8ms [-3.05%, -0.75%]
  •   :ballot_box_with_check:as_draws_rvars: 80.8ms -> 80.5ms [-1.36%, +0.63%]
  •   :ballot_box_with_check:summarise_draws_100_variables: 709ms -> 710ms [-0.11%, +0.41%]
  •   :ballot_box_with_check:summarise_draws_10_variables: 109ms -> 109ms [-0.97%, +0.56%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@n-kall n-kall marked this pull request as draft March 15, 2024 15:49
@n-kall
Copy link
Collaborator Author

n-kall commented Mar 15, 2024

I changed this to a draft as I am now thinking about whether a single relative_eff function is the way to go. Perhaps there should be one for different quantities (like ESS) or none at all

@n-kall
Copy link
Collaborator Author

n-kall commented May 10, 2024

As a single r_eff function doesn't really make sense (it should be dependent on the quantity of interest), I will close this PR.

@n-kall n-kall closed this May 10, 2024
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.

[Feature] Add relative_eff function
2 participants