Skip to content
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

Prevent LOO through vfold_cv() #527

Merged
merged 7 commits into from
Sep 9, 2024

Conversation

seb09
Copy link
Contributor

@seb09 seb09 commented Aug 15, 2024

vfold_cv() throws an error, if used for leave-one-out cross-validation, refering to loo_cv() instead. Fixes #440

vfold_cv() throws an error, if used for leave-one-out cross-validation, refering to loo_cv() instead.
Fixes tidymodels#440
@hfrick
Copy link
Member

hfrick commented Aug 23, 2024

@seb09 Thanks a bunch for the PR! I'm just dropping in quickly to say that I'm OOO until the week after next but will get to this after my return.

@seb09
Copy link
Contributor Author

seb09 commented Aug 23, 2024

Enjoy your time off 🙂

Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seb09 rsample already has a function to check that v is something sensible, check_v(), so I've moved your check there. This means we now catch it in vfold_cv() and clustering_cv().

R/vfold.R Outdated
@@ -74,17 +76,20 @@ vfold_cv <- function(data, v = 10, repeats = 1,
strata_check(strata, data)
check_repeats(repeats)

if (isTRUE(v == nrow(data))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice defensive isTRUE() 👍

R/vfold.R Outdated
Comment on lines 15 to 17
#' @param v The number of partitions of the data set. Should be an integer
#' smaller than `nrow(data)`. If you want to create a split for a leave-one-out
#' cross-validation (`v = nrow(data)`), please use [loo_cv()] instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also require a few other things of v, e.g. to be larger than 1 and don't mention it all here. We could move it to the Details section but I think it's okay to let users bump into the error for the edgecases so that we can keep their focus on more high-level aspects.

@hfrick hfrick merged commit e02d339 into tidymodels:main Sep 9, 2024
12 checks passed
Copy link

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.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent LOO through vfold_cv()
2 participants