Skip to content

refactor step_regex() and step_count() #1169

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

Merged
merged 3 commits into from
Jul 28, 2023
Merged

refactor step_regex() and step_count() #1169

merged 3 commits into from
Jul 28, 2023

Conversation

EmilHvitfeldt
Copy link
Member

Related to #1156.

This PR does a small refactor to step_regex() and step_count() to bring them up to the same standard as #1156.

Further, the steps new use check_name(), and will thus error in result is set to a name that is in the data set. Previously it would silently overwrite.

@EmilHvitfeldt EmilHvitfeldt mentioned this pull request Jul 27, 2023
12 tasks
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.

This is great!

A paper trail for future passersby: I was missing a bit of context while trying to understand how many of these lines transition eval_select() output object$input to names(object$input). I initially had trouble wrapping my head around how that was fully safe (e.g. depending on object$input being c(Species = "Species")), and this seemed to suggest to me not:

recipes/R/selections.R

Lines 204 to 208 in 59486ec

# Return names not positions, as these names are
# used for both the training and test set and their positions
# may have changed. If renaming is allowed, add the new names.
out <- names(data)[sel]
names <- names(sel)

This is fine for both step_count() and step_regex(), though, because both leave allow_rename = FALSE as default in recipes_eval_select() and don't surface that argument to the user.🐛

@EmilHvitfeldt EmilHvitfeldt merged commit 9aaa5d4 into main Jul 28, 2023
@github-actions
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 Aug 12, 2023
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