-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Rollups of summaries of variables with indices, version 2 #343
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #343 +/- ##
==========================================
+ Coverage 95.31% 95.78% +0.46%
==========================================
Files 50 51 +1
Lines 3840 3911 +71
==========================================
+ Hits 3660 3746 +86
+ Misses 180 165 -15 ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Thanks for picking this up and turning it into something production-ready! |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if fbe5543 is merged into master:
|
Thanks for getting it started way back when! It helped a lot. |
Summary
Now that some improved index-handling code is in, I thought I'd take a second attempt at rollups of summaries of variables with indices. This is a PR that will close #43 if it goes forward.
This is based on @jsocolar's #152, except I have iterated on the interface somewhat to be a bit more generic. It allows arbitrary rollup functions to be given on a per-original-summary-column basis, and supplies both overall default rollup functions and summary-specific rollup functions (e.g. having the ess functions rollup with min and rhat functions with max by default).
Demo:
That last bit about
NA
s is IMO the weakest part of the API at the moment. Not sure if it needs fixing. One option is to add an overallna.rm = TRUE
option to remove NAs before passing to the rollup functions. Could also allow NAs to be removed from specific summaries with a named argument; likena.rm = c(FALSE, ess_bulk = TRUE)
or something like that.Pinging folks who seemed interested in this from previous issues: @avehtari @paul-buerkner @jgabry @jsocolar @andrewgelman
Copyright and Licensing
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: