performance_cv() fails with method = "loo" (object ‘test_resd’ not found)#911
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the performance_cv function by modularizing its logic, adding metric validation, and implementing a fast Leave-One-Out (LOO) approximation for linear models. It also removes the as.data.frame.performance_cv S3 method, standardizes documentation links, and updates the test suite. Reviewer feedback highlights a style guide violation regarding warning formatting and an inconsistency between the documentation and validation logic for the ELPD metric.
| message(insight::color_text( | ||
| insight::format_message( | ||
| "Requested number of folds (k) larger than the sample size.", | ||
| "'k' set equal to the sample size (leave-one-out [LOO])." | ||
| ), | ||
| color = "yellow" | ||
| )) |
There was a problem hiding this comment.
According to the repository style guide, user-facing warnings should be issued using insight::format_warning(). This ensures consistent formatting and behavior across the easystats ecosystem. The current implementation using message() and insight::color_text() is less idiomatic for this project.
insight::format_warning(
"Requested number of folds (k) larger than the sample size.",
"'k' set equal to the sample size (leave-one-out [LOO])."
)References
- Use the insight package's functions for user-facing messages: For warnings: insight::format_warning() (link)
| #' leave-one-out (`"loo"`). If `data` is supplied, this argument is ignored. | ||
| #' @param metrics Can be `"all"`, `"common"` or a character vector of metrics to be | ||
| #' computed (some of `c("ELPD", "Deviance", "MSE", "RMSE", "R2")`). "common" will | ||
| #' computed (some of `"ELPD"`, `"MSE"`, `"RMSE"`, `"R2"`). `"common"` will |
There was a problem hiding this comment.
The documentation lists ELPD as a possible metric to be computed. However, the validation logic at line 59 explicitly excludes it from the allowed set, which will result in a 'not yet supported' error if a user tries to use it. To avoid confusion, consider removing ELPD from the documentation string until it is fully implemented, or update the validation set to allow it if the intention is to provide a specific 'not yet supported' message for this metric.
1 participant
Fixes #896