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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] Adding any and all support from libcudf #3094

Merged
merged 5 commits into from
Oct 16, 2019
Merged

[REVIEW] Adding any and all support from libcudf #3094

merged 5 commits into from
Oct 16, 2019

Conversation

rgsl888prabhu
Copy link
Contributor

@rgsl888prabhu rgsl888prabhu commented Oct 15, 2019 •
edited
Loading

Adding any and all API support from libcudf. Current approach is similar to the earlier version of using min and max.

closes #1874

@rgsl888prabhu rgsl888prabhu requested review from a team as code owners October 15, 2019 17:13
@rgsl888prabhu rgsl888prabhu self-assigned this Oct 15, 2019
@rgsl888prabhu rgsl888prabhu changed the title [WIP] Adding any and all support from libcudf [REVIEW] Adding any and all support from libcudf Oct 15, 2019
@codecov
Copy link

codecov bot commented Oct 15, 2019 •
edited
Loading

Codecov Report

Merging #3094 into branch-0.11 will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.11    #3094   +/-   ##
============================================
  Coverage        86.91%   86.91%           
============================================
  Files               49       49           
  Lines             9194     9194           
============================================
  Hits              7991     7991           
  Misses            1203     1203
Impacted Files Coverage Δ
python/cudf/cudf/core/column/numerical.py 94.34% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af7957a...2854e17. Read the comment docs.

@harrism
Copy link
Member

harrism commented Oct 16, 2019

@rgsl888prabhu Please don't add new functionality using the gdf_column / gdf_scalar as we're trying to replace them in 0.11

@harrism harrism requested a review from karthikeyann October 16, 2019 01:08
@harrism
Copy link
Member

harrism commented Oct 16, 2019

@karthikeyann can you please review since you are looking at porting reductions currently?

@harrism harrism added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Oct 16, 2019
shwina
shwina approved these changes Oct 16, 2019
View reviewed changes
Copy link
Contributor

@shwina shwina left a comment

Choose a reason for hiding this comment

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

Cython looks great!

@shwina shwina merged commit d2c5c99 into rapidsai:branch-0.11 Oct 16, 2019
karthikeyann
karthikeyann reviewed Oct 16, 2019
View reviewed changes
cpp/src/reductions/any.cu
#include "simple.cuh"

gdf_scalar cudf::reduction::any(gdf_column const& col, gdf_dtype const output_dtype, cudaStream_t stream)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

output_dtype should always be bool type.

@shwina
Copy link
Contributor

shwina commented Oct 16, 2019 •
edited
Loading

@karthikeyann I apologize. I missed @harrism's comment that asked for your review.

@harrism I think it's possible to explicitly request reviews via the GitHub UI which would have blocked this PR from being merged. Definitely my fault here as I should have paid more attention, especially after assuming the responsibility of merging. But it could avoid something like this from happening again.

@shwina
Copy link
Contributor

shwina commented Oct 16, 2019

Perhaps @karthikeyann's additional review comments can be addressed in a follow up PR, @rgsl888prabhu?

Sorry for the trouble :(

@karthikeyann
Copy link
Contributor

@shwina No problem. Porting gdf_scalar to cudf::scalar will be an another PR. Any comments could be addressed in that follow up PR.

@shwina
Copy link
Contributor

shwina commented Oct 16, 2019

Sounds good. Thanks!

@rgsl888prabhu
Copy link
Contributor Author

Thank you @karthikeyann @shwina

@harrism
Copy link
Member

harrism commented Oct 17, 2019

@shwina I did request review using the UI, and it didn't block merging. @karthikeyann can you please file an issue to make sure your comment doesn't get forgotten?

@vyasr vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuDF (Python) Reviewer labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karthikeyann karthikeyann karthikeyann left review comments

@shwina shwina shwina approved these changes

@harrism harrism harrism approved these changes

Assignees

@rgsl888prabhu rgsl888prabhu

Labels
4 - Needs Review Waiting for reviewer to review or respond libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Add libcudf any() and all() reductions
5 participants
@rgsl888prabhu @harrism @shwina @karthikeyann @vyasr