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

New subclasses and inner_split() methods for validation sets #489

Merged
merged 8 commits into from
May 24, 2024

Conversation

hfrick
Copy link
Member

@hfrick hfrick commented May 23, 2024

This PR adds inner_split() methods for the rsets coming from validation_set(). Those rsets can result from the group and time variants of initial_validation_split().

To be able to tell where they are coming from, this PR also adds subclasses for the group and time variant to the output of validation_set(): the rset objects and the rsplit objects within those rsets.

I'm looking for a second opinion on the naming of the rsplit subclasses for the time variant.

The interfaces and the original (and deprecated) approach via validation_split() put the time neither as a prefix nor a suffix but rather inside the more general name: initial_time_split(), initial_validation_time_split(), validation_time_split(), and val_time_split.

I don't think we are going to touch the naming of the interfaces but we could touch the naming convention for the rsplit class. I've broken with that particular pattern of placing time and instead aligned with the pattern for group, i.e., used it as a prefix. Could be good, could be bad, could actually not matter much.

The only place I think it'll matter is here and I'm happy to add a method for val_time_split to support the usage of the deprecated validation_time_split() if we think it's necessary.
Edit: this search suggests that we don't dispatch on that class elsewhere so it should only matter here.

@hfrick hfrick changed the title inner_split() methods for validation sets New subclasses and inner_split() methods for validation sets May 23, 2024
@hfrick hfrick requested a review from simonpcouch May 24, 2024 13:07
Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

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

yup yup yup yup

I'm happy to add a method for val_time_split to support the usage of the deprecated validation_time_split() if we think it's necessary.

I'd say we not do so. :)

R/inner_split.R Show resolved Hide resolved
R/inner_split.R Outdated Show resolved Hide resolved
R/misc.R Show resolved Hide resolved
R/inner_split.R Show resolved Hide resolved
R/inner_split.R Show resolved Hide resolved
@hfrick hfrick merged commit 3d8b275 into main May 24, 2024
12 checks passed
@hfrick hfrick deleted the inner_split-validation-set branch May 24, 2024 15:44
Copy link

github-actions bot commented Jun 8, 2024

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 Jun 8, 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.

2 participants