Repository navigation
Add classes to all loo warnings - #417
florence-bockting wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #417 +/- ##
==========================================
+ Coverage 92.65% 92.95% +0.30%
==========================================
Files 31 31
Lines 3240 3238 -2
==========================================
+ Hits 3002 3010 +8
+ Misses 238 228 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if febdcbc is merged into master:
|
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
Looks good. I made a few suggestions, but pretty small.
| if (any(tail_len_bad)) { | ||
| if (length(tail_lengths) == 1) { | ||
| warning( | ||
| .warn_diagnostic( |
There was a problem hiding this comment.
The original warning() calls here used immediate. = TRUE, which we added on purpose in a376936. do_importance_sampling() calls this before the PSIS loop, so we made it so that users see the warning right away instead of waiting for a potentially very long computation.
But rlang::warn() doesn't seem to have an immediate. argument, so now the warning will only show up after the whole call finishes (and same for the else branch below).
I think we should keep the immediate behavior by temporarily setting options(warn = 1). The .warn function would change to something like this:
.warn <- function(..., class = NULL, immediate = FALSE) {
if (immediate && getOption("warn") < 1) {
rlang::local_options(warn = 1)
}
rlang::warn(paste(c(...), collapse = ""), class = c(class, "loo_warning"))
}and then we can pass immediate = TRUE here via .warn_diagnostic since it will get passed along via ....
What do you think?
| bad <- which(tail_len_bad) | ||
| Nbad <- length(bad) | ||
| warning( | ||
| .warn_diagnostic( |
There was a problem hiding this comment.
Same comment as above about immediate
| #' | ||
| #' @section Warning classes: | ||
| #' | ||
| #' Every warning signaled by \pkg{loo} has the class `loo_warning` and one |
There was a problem hiding this comment.
This isn't 100% true yet because there are still three .Deprecated() calls (in psislw(), in compare(), and for the log_q argument of psis_approximate_posterior()). Those warnings have the class deprecatedWarning but not loo_warning. Should we convert them to .warn(..., class = "loo_warning_deprecated") too? Or maybe we don't need to change those since we can remove them in loo v3.0 because psislw and compare are already deprecated?
| * Fix `crps()` and `scrps()` ignoring the `permutations` argument for numeric | ||
| input by @thanasibakis in #398 | ||
| * Speed up the Bayesian bootstrap in `pseudobma_weights()` by @VisruthSK in #368 | ||
| * All warnings now have the class `loo_warning` and a class for each cause; |
There was a problem hiding this comment.
I think it's probably worth also mentioning that the base warning class changes to rlang_warning. I'm guessing it doesn't affect anyone, but just in case.
| #' * `loo_warning_subsample_se`: A subsampling standard error is naive or | ||
| #' negative. |
There was a problem hiding this comment.
This class seems to cover two different causes: "Naive diff SE is used" from loo_compare() with subsampling, and "Negative estimate of SE" from loo_subsample(). In the posterior PR each cause got its own class. Should we split this into two classes, e.g. loo_warning_subsample_naive_se and loo_warning_subsample_negative_se? Just a thought, I don't really think it's a big deal either way.
| ) | ||
| }) | ||
|
|
||
| test_that("warning helpers signal their classes", { |
There was a problem hiding this comment.
These tests cover the helpers and the throw_*() functions, but not the places that call .warn() or .warn_diagnostic() directly, e.g. the loo_compare() checks, subsampling,do_psis_i(), and others. Should we add a few tests through user-facing functions? I don't think they all need tests, but a few tests would be a sanity check that this functionality is working properly. For example, maybe loo_compare() with mismatched yhash and one or two others?
| } else { | ||
| warning("Negative estimate of SE, more subsampling obs. needed.", call. = FALSE) | ||
| .warn_diagnostic("Negative estimate of SE, more subsampling obs. needed.", class = "loo_warning_subsample_se") | ||
| x$estimates["elpd_loo", "SE"] <- NaN |
There was a problem hiding this comment.
Not related to this PR, but I noticed it while reviewing: this line 1095 is in the p_loo branch, so it should be x$estimates["p_loo", "SE"] <- NaN (p_loo not elpd_loo). Could you just fix it here since you're already editing these lines? I could open a separate issue and PR, but it would be super easy to just fix it here.
Description
This PR adds a class to each loo warning, following up on our corresponding work in the
posteriorpackage: stan-dev/posterior#470.loo_warningand a class for its cause.loo_warning_diagnostic.R/loo-package.RrlangtoImports.throw_psis_r_eff_warning()andthrow_loo_r_eff_warning()(this was dead code)This PR is related to Issue #353 (but does not fully solve it).
The PR was assisted by an LLM.