Skip to content

ARROW-12185: [R] Bindings for any, all - #10032

Closed
thisisnic wants to merge 6 commits into
apache:masterfrom
thisisnic:ARROW-12185-any_all
Closed

ARROW-12185: [R] Bindings for any, all#10032
thisisnic wants to merge 6 commits into
apache:masterfrom
thisisnic:ARROW-12185-any_all

Conversation

@thisisnic

Copy link
Copy Markdown
Member

No description provided.

@github-actions

Copy link
Copy Markdown

Comment thread r/R/compute.R Outdated
Comment thread r/R/compute.R Outdated
Comment on lines +358 to +362
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))

@ianmcook ianmcook Apr 19, 2021

Copy link
Copy Markdown
Member

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.:

Suggested change
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)

@ianmcook ianmcook Apr 19, 2021

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, updated!

@ianmcook

Copy link
Copy Markdown
Member

@thisisnic it looks like when you ran devtools::document(), it built some docs changes unrelated to this PR. This happens sometimes (e.g. when someone else commits a PR without running devtools::document()) and it's no big deal, but it'd be a good idea to push a commit to undo those unrelated docs changes, to avoid any merge conflicts.

@thisisnic
thisisnic force-pushed the ARROW-12185-any_all branch from 6f81a57 to defeac1 Compare April 20, 2021 08:29
@thisisnic
thisisnic force-pushed the ARROW-12185-any_all branch from defeac1 to 12cb6cb Compare April 20, 2021 08:34
@thisisnic

Copy link
Copy Markdown
Member Author

@thisisnic it looks like when you ran devtools::document(), it built some docs changes unrelated to this PR. This happens sometimes (e.g. when someone else commits a PR without running devtools::document()) and it's no big deal, but it'd be a good idea to push a commit to undo those unrelated docs changes, to avoid any merge conflicts.

Fixed now!

@nealrichardson nealrichardson left a comment

Copy link
Copy Markdown
Member

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.

Comment thread r/R/compute.R Outdated
Comment thread r/R/compute.R Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants