-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adds ys_prune #97
Adds ys_prune #97
Conversation
spec <- ys_help$spec() | ||
data$STUDY <- NULL | ||
data$TAD <- NULL | ||
ans <- ys_prune(data, spec) |
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.
should this test have a data frame with more columns than the spec?
For example
- data has columns: A, B, C
- spec has columns: A, B
The current test has the reverse, and therefore:
names(ys_prune(data, spec)) == names(data)
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.
Yes; I was intending fewer columns. Can add more columns too.
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.
Added two columns matching nothing.
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.
One other question - if I add:
data <- data[, rev(names(data)), drop = FALSE]
after line 12 (data$BAR <- 2
) but before line 13 (ans <- ys_prune(data, spec))
), the test then fails.
Is this OK?
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.
Thanks, @andersone1 ; yes, there was something wrong in the logic for matching / ordering the names.
- Fixed the logic in
ys_prune
- Randomized the names in
data
in the test - Added expectations for reporting columns not there
Thanks!
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 reverted that one expectation; I do think we want this to be the check:
expect_equal(names(ans), names(spec2))
The names in the final answer are identical (including order) to the names in the spec, minus the columns that we dropped.
Summary
spec
objectspec
spec
that aren't indata
data
andspec