Add outcome_names() method for workflows using add_variables()#994
Add outcome_names() method for workflows using add_variables()#994
Conversation
hfrick
left a comment
hfrick
left a comment
There was a problem hiding this comment.
I'm missing some of the context in which this is (going to be) called, so I got two questions :D
|
|
||
| #' @export | ||
| #' @rdname outcome_names | ||
| outcome_names.tune_results <- function(x, ..., call = caller_env()) { |
There was a problem hiding this comment.
Do we need this change in function signature for tune_results objects? 🤔
There was a problem hiding this comment.
You're welcome to suggest a change.
| cli::cli_abort( | ||
| "To determine the outcome names when {.fn add_variables} is used, please | ||
| pass the training set data to the {.arg data} argument.", | ||
| call = call | ||
| ) | ||
| } |
There was a problem hiding this comment.
Is this an error message for the user or for us?
There was a problem hiding this comment.
Okay, digging around a bit more here and I think there is a little messaging mismatch to resolve in that error message:
Do you mean that the user of outcome_names() should use the data argument of outcome_names()? The user of outcome_names() is presumably just us since outcome_names() is labeled with @keywords internal. In that case, I wouldn't pass in the call to cli_abort() and possibly remove the reference to add_variables(). The context of how we got into this probably lives better elsewhere, likely wherever we call it from and need to add data, so in tune_grid() or int_pctl().
If you mean the general tidymodels user the current error message is probably confusing: they only call tune_grid() or int_pctl() and couldn't use that data argument to outcome_names(). tune_grid() does not have a data argument and int_pctl() has .data but that is not the same as the data for outcome_name(). (And imagining a very desperate user, looking into the mentioned add_variables() also does not reveal a data argument.)
I'm guessing path 1 is the right one here?
|
|
||
| #' @export | ||
| #' @rdname outcome_names | ||
| outcome_names.tune_results <- function(x, ..., call = caller_env()) { |
There was a problem hiding this comment.
| outcome_names.tune_results <- function(x, ..., call = caller_env()) { | |
| outcome_names.tune_results <- function(x, ...) { |
I don't think the addition is necessary (and tests pass without it) so I'd just leave things as is.
| cli::cli_abort( | ||
| "To determine the outcome names when {.fn add_variables} is used, please | ||
| pass the training set data to the {.arg data} argument.", | ||
| call = call | ||
| ) | ||
| } |
There was a problem hiding this comment.
Okay, digging around a bit more here and I think there is a little messaging mismatch to resolve in that error message:
Do you mean that the user of outcome_names() should use the data argument of outcome_names()? The user of outcome_names() is presumably just us since outcome_names() is labeled with @keywords internal. In that case, I wouldn't pass in the call to cli_abort() and possibly remove the reference to add_variables(). The context of how we got into this probably lives better elsewhere, likely wherever we call it from and need to add data, so in tune_grid() or int_pctl().
If you mean the general tidymodels user the current error message is probably confusing: they only call tune_grid() or int_pctl() and couldn't use that data argument to outcome_names(). tune_grid() does not have a data argument and int_pctl() has .data but that is not the same as the data for outcome_name(). (And imagining a very desperate user, looking into the mentioned add_variables() also does not reveal a data argument.)
I'm guessing path 1 is the right one here?
2 participants
Closes #993