Conversation
|
Thanks for the PR! I haven't had a chance to look at the code yet (I'm a bit swamped with work at the moment), but a couple quick comments:
|
|
@jgabry I fixed the error that popped up. Well the color "issue" can be resolved quite simply if we switch from "mid" to "light" for PPD, like PPC. |
@tjmahr Any preference on this? The first option would keep the code simpler, but it would change the appearance of all the PPD plots (could we call this breaking visual backwards compatibility?). I would lean towards the second option but I'm not really sure. |
|
I just tried using this branch and |
|
Sorry, it was only documented in |
I did this - wasn't too hard, and it actually made some of the code more simple. I've updated the examples above - I think it looks much better (note that the first |
|
Thanks, I agree it looks better! |
|
I think after that change there are now some visual tests that are failing. I checked one of the ppd_stat ones and it looks like it's the legend that's changed. |
|
Okay, should be fixed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #429 +/- ##
==========================================
- Coverage 98.66% 96.83% -1.83%
==========================================
Files 35 35
Lines 5857 6010 +153
==========================================
+ Hits 5779 5820 +41
- Misses 78 190 +112 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Ok I finally took a closer look. I think this is getting close! I made two small review comments inline, and here are few other things I noticed:
- When
discrete=TRUE,ppd_stat(..., discrete = TRUE, show_marginal = TRUE)seems to behave strangely. It looks to me like it's just plotting a bar with a count of 1 instead of the marginal PPD, but I'm not entirely sure. For example try:
ypred <- matrix(rbinom(100 * 20, 1, 0.2), nrow = 100)
prop0 <- function(x) mean(x == 0)
ppd_stat(ypred, stat = prop0, discrete = TRUE, show_marginal = TRUE)
-
The default for
freqwas changed inppd_stat()but notppd_stat_grouped() -
Currently there are no tests that turn on
show_marginal=TRUE(this is also why the codecov comment shows low test coverage for the PR). Can you add some snapshot visual tests withshow_marginal = TRUE?
Also @avehtari and @tjmahr do either of you have any thoughts on this PR before we go ahead with it?
| show_marginal = show_marginal, | ||
| # in case user turns legend back on | ||
| guide = guide_legend( | ||
| override.aes = list(size = 2 * size, alpha = 1) |
There was a problem hiding this comment.
| override.aes = list(size = 2 * size, alpha = 1) | |
| override.aes = list(linewidth = 2 * size, alpha = 1) |
| fill = "PPD"), | ||
| notch = notch, | ||
| linewidth = 1, | ||
| outlier.color = get_color("mh"), |
There was a problem hiding this comment.
Should this be get_color("dh") now that we changed to the light and dark instead of middle colors? Or are the outliers intentionally using a different color?
Addresses #425
This PR adds the
show_marginalargument (defaultFALSE) to thePPD-distributionsand thePPD-test-statisticsfunctions (thePPD-intervalsfunctions de-facto show the marginal PPD(s)).I think we need a better default PPD color - which?
Here are all of these functions (default plots --
show_marginal=FALSE-- are unchanged!):ppd-distributions
ppd-test-statistics
Created on 2026-03-12 with reprex v2.1.1