Repository navigation
Add condition classes to all posterior warnings - #470
florence-bockting wants to merge 10 commits into
Conversation
Class names for the 17 warning_no_call() call sitesRule:
|
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2da45f4 is merged into master:
|
jgabry
left a comment
There was a problem hiding this comment.
Thanks @florence-bockting, I think this is good. I made a few small comments. One question: should we list the class names somewhere users can find them, for example a short section in ?posterior-package or a separate doc file? I'm not sure. Right now a maintainer of a package that depends on posterior would have to grep R/ to find e.g. posterior_warning_ess_capped. What do you think?
| warning_no_call <- function(...) { | ||
| warning(..., call. = FALSE) | ||
| warning_no_call <- function(..., class = NULL) { | ||
| rlang::warn(paste0(...), class = c(class, "posterior_warning")) |
There was a problem hiding this comment.
This is ok, but there's something we should keep in mind for when we add .frequency = "once", since it interacts with the handlers that are enabled in this PR. I checked with Claude and then check myself and once a warning has been shown, rlang doesn't signal it again at all, so a handler like the one in the PR description only sees the first one. Also, a muffled warning still counts as shown. If loo muffles the first ESS-capped warning, the user won't see it again that session, even when calling posterior directly.
So maybe we'll want .frequency only on the noisy warnings (ESS capped, the Pareto ones) rather than inside warning_no_call() for every call site.
There was a problem hiding this comment.
Thanks, I actually didn't thought about using .frequency = "once". I rather thought to use a corresponding caller at the sites where a posterior function is called multiple times (e.g., loo package) . This caller then collects the warning by class and reports a summary, for example "ESS capped in 12 of 100 calls". Then loo's handlers see every warning. Or do I misunderstand something?
There was a problem hiding this comment.
You're right, and I think your approach is better for the loo case. loo can collect warnings by class and report one summary, and it sees every warning. .frequency = "once" would conflict with that because after the first warning rlang stops signaling them and loo wouldn't be able to count them.
Users calling posterior directly might want once per session warnings, but we could add that in the future if we want it for certain specific warnings only. If we do that then loo could use rlib_warning_verbosity = "verbose" in those cases.
But for now fine to go with your approach.
Thank you. ?posterior-package has a new section "Warning and error classes". I also added the alias posterior_warning, so ?posterior_warning opens that page. |
jgabry
left a comment
There was a problem hiding this comment.
A few tiny suggestions on the new documentation, otherwise looks good!
| #' Every warning signaled by \pkg{posterior} has the class | ||
| #' `posterior_warning` and one class for its cause. Use these classes to | ||
| #' handle or muffle a specific warning, for example with | ||
| #' [withCallingHandlers()] and [invokeRestart()]`("muffleWarning")`. |
There was a problem hiding this comment.
This renders strangely for me in the help page. For the second one I think just skip the link and make it invokeRestart("muffleWarning") instead. [withCallingHandlers()] already correctly links to the same page anyway.
| #' `superchain_ids` does not equal the number of chains. | ||
| #' * `posterior_warning_superchain_unequal`: The superchains do not have the | ||
| #' same number of chains. | ||
| #' * `posterior_warning_non_numeric`: A conversion dropped non-numeric |
There was a problem hiding this comment.
I don't think the variables are actually dropped, just converted right? The warning itself just says "Converting non-numeric variables to numeric". So maybe: "Non-numeric variables (e.g., factors) were converted to numeric."
| #' * `posterior_warning_pareto_invalid_input`: The input to Pareto smoothing | ||
| #' contains infinite or `NA` values. |
There was a problem hiding this comment.
I think this warning also happens when the input is constant or has a constant tail. So maybe: "The input to Pareto smoothing contains infinite or NA values, is constant, or has a constant tail."
Great, thank you for review. I updated the docs corresponding to your suggestions. |
Closes #309
Summary
Packages such as
loocall posterior functions many times. Each call can signal the same warning, and the caller cannot catch one warning type selectively.warning_no_call()now signals withrlang::warn()and gives each warning two classes: the shared classposterior_warningand a class for the cause, for exampleposterior_warning_ess_capped.All 17 call sites pass a cause class (see comment below). The message text and the missing call stay the same.
A caller can now count and muffle one warning type:
Side effect:
The base class changes from
simpleWarningtorlang_warning. A handler forsimpleWarningno longer catches posterior warnings. A handler forwarningstill catches them.Tests:
The new tests in
test-convergence.Rcheck both classes on the ESS warning. They also check that a handler counts and muffles the warning. The project uses testthat edition 2, which ignoresexpect_warning(class = ), sothe tests use
tryCatch()withexpect_s3_class().Documentation:
The user can look up the warning classes through
?posterioror?posterior_warning.AI assistance:
AI assistance was used to draft the PR. I reviewed every change and can explain it.
Copyright and Licensing
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: