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

Grouped resamples are inconsistently of a group_* class #318

Closed
mikemahoney218 opened this issue Jun 29, 2022 · 4 comments · Fixed by #325
Closed

Grouped resamples are inconsistently of a group_* class #318

mikemahoney218 opened this issue Jun 29, 2022 · 4 comments · Fixed by #325

Comments

@mikemahoney218
Copy link
Member

After #315 and #316, we'll have four separate grouped resampling functions:

This is a lot of variation, and as Julia mentioned in #315, we should investigate a way to make this be more consistent.

Personally, I like the maximalist position I accidentally wound up with in group_bootstraps(): grouped resamples should subclass the non-grouped version, keeping all of their classes and adding a group_ class on top. I can't think of any case where a method should dispatch on, e.g., vfold_cv() but error on grouped outputs. The closest I can think of is #79, but in that situation you should provide a group_vfold_cv() method as well, not just error.

@juliasilge
Copy link
Member

Let's remember to check and update the classes in compat-vctrs.R and compat-vctrs-helpers.R as part of this.

@mikemahoney218
Copy link
Member Author

mikemahoney218 commented Jun 30, 2022

I'm wondering if we should subclass rset as well, so that all grouped resamples produce an object with classes c("group_rset", "rset"). That would let functions like what's proposed in #79 error on all grouped objects as a class, rather than have it be specific to each object type. Might be too much future-proofing though.

@juliasilge
Copy link
Member

Once we get into this, let's see if that is possible and manageable, because doing it all at once is more likely to end up solving our inconsistency challenges, I think. Definitely a lower priority than at least getting us consistent, though.

@github-actions
Copy link

This issue 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 Jul 16, 2022
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 a pull request may close this issue.

2 participants