add collect_metrics() argument to pivot output
#839
Conversation
| @@ -11,17 +11,26 @@ | |||
| #' used to filter the predicted values before processing. This tibble should | |||
| #' only have columns for each tuning parameter identifier (e.g. `"my_param"` | |||
| #' if `tune("my_param")` was used). | |||
| #' @param type One of `"long"` (the default) or `"wide"`. When `type = "long"`, | |||
There was a problem hiding this comment.
I'm conflicted on the name and values for this argument. Very much open to suggestions. :)
There was a problem hiding this comment.
I think that a logical called pivot_wider is good. It implies that it is already long.
There was a problem hiding this comment.
Just to check, we decided that it's unlikely that we'll ever deviate from something binary? No other forms of "wide" metrics anticipated?
| #' output has columns `.metric` and one of `.estimate` or `mean`. | ||
| #' `.estimate`/`mean` gives the values for the `.metric`. When `type = "wide"`, | ||
| #' each metric has its own column and the `n` and `std_err` columns are removed, | ||
| #' if they exist. |
There was a problem hiding this comment.
I didn't have strong opinions here and the original implementation in #689 had the same behavior, so I did it this way, but if we wanted we could have columns like rmse, rmse_n, rmse_std_err.
There was a problem hiding this comment.
I think excluding them is fine; we can add them later if someone asks for them.
| if (inherits(x, "last_fit")) { | ||
| return(x$.metrics[[1]]) | ||
| res <- x$.metrics[[1]] | ||
| } else { |
There was a problem hiding this comment.
Not stoked about this design pattern.😞 But wasn't clear to me that there's a super clean way to write this.
|
finetune has its own |
|
Ah, I had forgotten that |
| @@ -11,17 +11,26 @@ | |||
| #' used to filter the predicted values before processing. This tibble should | |||
| #' only have columns for each tuning parameter identifier (e.g. `"my_param"` | |||
| #' if `tune("my_param")` was used). | |||
| #' @param type One of `"long"` (the default) or `"wide"`. When `type = "long"`, | |||
There was a problem hiding this comment.
Just to check, we decided that it's unlikely that we'll ever deviate from something binary? No other forms of "wide" metrics anticipated?
I've thought about it some more and I don't think that we will have more ways of pivoting (in tune, at least). characterize might. It is conceivable that characterize would have up to three options for pivoting but I think that its API would be consistent with the logical/binary argument ☝️ |
|
The only other form of "wide" or "long" I could imagine is one a la |
|
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. |
A follow-up on #689.
Still needs tests for survival and finetuning, to come in extratests. Opening as draft as I haven't began writing those tests.