Repository navigation
Signal each class of posterior warning once #416
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -280,3 +280,23 @@ release_questions <- function() { | |
| is_constant <- function(x, tol = .Machine$double.eps) { | ||
| abs(max(x) - min(x)) < tol | ||
| } | ||
|
|
||
| #' Collect posterior warnings and signal each class once | ||
| #' | ||
| #' @noRd | ||
| #' @param expr Expression to evaluate. | ||
| #' @return The value of `expr`. | ||
| .collect_warnings <- function(expr) { | ||
| collected <- list() | ||
| out <- withCallingHandlers( | ||
| expr, | ||
| posterior_warning = function(cnd) { | ||
| collected[[class(cnd)[1]]] <<- cnd | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this keeps the last warning of each class, not the first. The only class this really affects is pareto_tail_exceeds_draws, since those messages include actual numbers. I'm not sure that this really matters, though. What do you think? |
||
| invokeRestart("muffleWarning") | ||
| } | ||
| ) | ||
| for (cnd in collected) { | ||
| warning(cnd) | ||
| } | ||
| out | ||
| } | ||
|
Comment on lines
+289
to
+302
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if the function the user calls errors for whatever reason they won't see the warnings. What about this slightly modified version that moves the loop into an .collect_warnings <- function(expr) {
collected <- list()
on.exit(for (cnd in collected) warning(cnd))
withCallingHandlers(
expr,
posterior_warning = function(cnd) {
collected[[class(cnd)[1]]] <<- cnd
invokeRestart("muffleWarning")
}
)
} |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,3 +91,29 @@ test_that("loo_cores works", { | |
| expect_warning(expect_equal(loo_cores(10), 2), "deprecated") | ||
| options(loo.cores = NULL) | ||
| }) | ||
|
|
||
| test_that(".collect_warnings() signals each posterior class once", { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With CRAN posterior 1.7.1 there are no warning classes yet, so in CI no posterior warning ever gets to I think this test for the helper itself is good and we should keep it, but maybe add some tests that the functions users call actually signal each warning only once? How about adding these two tests?
|
||
| warn_classed <- function(class) { | ||
| warning(structure( | ||
| class = c(class, "posterior_warning", "warning", "condition"), | ||
| list(message = class, call = NULL) | ||
| )) | ||
| } | ||
| caught <- character() | ||
| out <- withCallingHandlers( | ||
| .collect_warnings({ | ||
| warn_classed("a") | ||
| warn_classed("a") | ||
| .collect_warnings(warn_classed("b")) | ||
| warn_classed("b") | ||
| warning("other") | ||
| 1 | ||
| }), | ||
| warning = function(cnd) { | ||
| caught <<- c(caught, conditionMessage(cnd)) | ||
| invokeRestart("muffleWarning") | ||
| } | ||
| ) | ||
| expect_equal(out, 1) | ||
| expect_equal(caught, c("other", "a", "b")) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this only applies if the user has the GitHub version of posterior. Should we mention that it's conditional on posterior > 1.7.1?