-
Notifications
You must be signed in to change notification settings - Fork 66
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
Standardize classing for grouped resampling #325
Conversation
@@ -195,13 +195,13 @@ group_vfold_cv <- function(data, group = NULL, v = NULL, balance = c("groups", " | |||
|
|||
## Save some overall information | |||
|
|||
cv_att <- list(v = v, group = group, balance = balance) | |||
cv_att <- list(v = v, group = group, balance = balance, repeats = 1, strata = FALSE) |
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.
This is a bit extraneous for this PR, but as part of a future PR (likely to address #79) I want to try and make sure that group_*
variants always have all the same attributes as their non-grouped superclass, so that methods dispatching on the non-grouped variant can always handle the grouped variant as well.
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've got one question on printing, and then I was wondering if we could write a test to loop through all the relevant rset types and do this, like you posted:
all(class(mc$splits[[1]]) %in% class(gmc$splits[[1]]))
R/printing.R
Outdated
pretty.group_validation_split <- function(x, ...) { | ||
details <- attributes(x) | ||
res <- paste0( | ||
"Grouped Validation Set Split (", |
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.
Do you think we should print "Grouped" or "Group"? We have both right now.
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.
Hmm -- I'm maybe 60/40 in favor of "Group", because the output isn't "Grouped" in a dplyr sense.
Do we want to print either, though? Is it ever important to know in interactive use that these splits were made using groups? Programmatic use will look for the group_*
classes.
Between the three, I think my favorite is to always prefix with "Group", but it's an extremely weak opinion.
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.
Yeah, let's keep the special printing for now and go with "Group". 👍
Alright, added tests and changed printing in 43c778e 😄 |
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.
Perfect! Thank you so much 🙌
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. |
This PR fixes #318 by standardizing how we subclass when performed grouped resampling. Grouped resampling now:
group_rset
, andgroup_x
class to the rset objectgroup_x
class to the rsplit objectCreated on 2022-06-30 by the reprex package (v2.0.1)