-
Notifications
You must be signed in to change notification settings - Fork 42
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
catch and log extract
issues
#575
Conversation
@@ -66,6 +66,7 @@ summarize_notes <- function(x) { | |||
dplyr::group_nest(type) %>% | |||
dplyr::mutate(data = purrr::map(data, ~ dplyr::count(.x, note))) %>% | |||
tidyr::unnest(data) %>% | |||
dplyr::rowwise() %>% |
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.
The changes in this file fix a bug in summarize_notes()
where the condition messages for warnings and errors were pasted if there were multiple distinct notes. I indulged myself and went ahead and made this fix so that we could snapshot test that case well, but committed those changes separately so that we could document what that test output looks like without those changes.
Message | ||
x Bootstrap1: preprocessor 1/1, model 1/1 (extracts): Error in extractor(object): AHHH | ||
x Bootstrap2: preprocessor 1/1, model 1/1 (extracts): Error in extractor(object): AHHH | ||
x Bootstrap3: preprocessor 1/1, model 1/1 (extracts): Error in extractor(object): AHHH |
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.
I'd like to make these Error in extractor(object)
bits of the error messages more informative, but that would require changes to more general machinery (and many snapshot updates) that would be better off living in a separate PR.
R/pull.R
Outdated
) | ||
) | ||
extracts <- dplyr::bind_cols(grid, labels(split)) | ||
extracts$.extracts <- list(extract_details(workflow, ctrl$extract)) |
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.
mutate()
wraps the raised errors with information that, when collapsed by tune's machinery, obscures the condition messages raised from the extractor functions themselves.
* delete unneeded `workflow` argument * restores former `pulley` behavior * correct spelling of `control` argument
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.
Looks good!
Co-authored-by: Davis Vaughan <davis@rstudio.com>
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
With CRAN tune, the package hides errors that occur with mis-specified
extract
functions:Created on 2022-11-04 with reprex v2.0.2
With this PR, the package surfaces
extract
ion errors in the same way that it does errors that occur elsewhere:Created on 2022-11-04 with reprex v2.0.2
Fixes #565, to the extent that we will. :)