-
Notifications
You must be signed in to change notification settings - Fork 3.9k
ARROW-12185: [R] Bindings for any, all #10032
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
Conversation
| array_data <- Array$create(data) | ||
|
|
||
| expect_equal(as.vector(any(array_data > 5)), any(data > 5)) | ||
| expect_equal(as.vector(any(array_data < 1)), any(data < 1)) | ||
| expect_equal(as.vector(any(array_data < 1, na.rm = TRUE)), any(data < 1, na.rm = TRUE)) |
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.
These tests could be simplified by using the helper function expect_vector_equal() (which is defined in helper-expectation.R). E.g.:
| array_data <- Array$create(data) | |
| expect_equal(as.vector(any(array_data > 5)), any(data > 5)) | |
| expect_equal(as.vector(any(array_data < 1)), any(data < 1)) | |
| expect_equal(as.vector(any(array_data < 1, na.rm = TRUE)), any(data < 1, na.rm = TRUE)) | |
| expect_vector_equal(any(input > 5), data) | |
| expect_vector_equal(any(input < 1), data) | |
| expect_vector_equal(any(input < 1, na.rm = TRUE), 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.
Note that expect_vector_equal() tests both Array and ChunkedArray so you'll be able to reduce the number of tests used here
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, updated!
|
@thisisnic it looks like when you ran |
6f81a57 to
defeac1
Compare
defeac1 to
12cb6cb
Compare
Fixed 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.
Looks good! I'm going to add some inline comments to explain the three-valued logic (as well as tag @rok about doing it correctly in C++), then merge.
No description provided.