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.

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

[REVIEW] Adding support for is_null and is_not_null #2962

Merged

Conversation

rgsl888prabhu
Copy link
Contributor

Checks the input column for null values, and creates a bool column of same size with true representing null values and false for other - is_null

Checks the input column for null values, and creates a bool column of same size with false representing null values and true for other - is_not_null

close #2143

@rgsl888prabhu rgsl888prabhu requested review from a team as code owners October 3, 2019 22:25
@harrism
Copy link
Member

harrism commented Oct 4, 2019

Now that cudf::column (#2207) is merged, and since these operators should work the same on any kind of column (since they just operate on the null mask), it would be nice to implement these to operate on cudf::column since eventually gdf_column will be removed. Otherwise this is just adding technical debt.

@rgsl888prabhu can you look into changing the APIs to use cudf::column?

Edit: hmmm, this would depend on porting cudf::fill() first (#2936 ). So perhaps it should wait.

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Nice and simple PR. Can simplify it more with Thrust. Also some comment suggestions.

cpp/tests/unary/unary_ops_test.cu Outdated Show resolved Hide resolved
cpp/include/cudf/unary.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/unary.hpp Outdated Show resolved Hide resolved
cpp/src/unary/null_ops.cu Outdated Show resolved Hide resolved
@rgsl888prabhu rgsl888prabhu requested a review from a team as a code owner October 4, 2019 16:10
@rgsl888prabhu rgsl888prabhu self-assigned this Oct 4, 2019
@rgsl888prabhu rgsl888prabhu changed the title [WIP] Adding support for isNULL and isNotNULL [REVIEW] Adding support for isNULL and isNotNULL Oct 4, 2019
@rgsl888prabhu rgsl888prabhu added 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer 4 - Needs Review Waiting for reviewer to review or respond labels Oct 4, 2019
@codecov
Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #2962 into branch-0.11 will increase coverage by 0.07%.
The diff coverage is 85%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.11    #2962      +/-   ##
===============================================
+ Coverage        86.95%   87.02%   +0.07%     
===============================================
  Files               49       49              
  Lines             9211     9194      -17     
===============================================
- Hits              8009     8001       -8     
+ Misses            1202     1193       -9
Impacted Files Coverage Δ
python/cudf/cudf/utils/cudautils.py 48.04% <ø> (-0.63%) ⬇️
python/cudf/cudf/core/series.py 93.8% <100%> (-0.03%) ⬇️
python/cudf/cudf/core/column/column.py 87.19% <75%> (-0.14%) ⬇️
python/cudf/cudf/core/index.py 89.97% <87.5%> (-0.05%) ⬇️

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 7cff6cc...6d85384. Read the comment docs.

@rgsl888prabhu
Copy link
Contributor Author

rerun tests

@rgsl888prabhu
Copy link
Contributor Author

@harrism

Copy link
Contributor

@abellina abellina left a comment

Choose a reason for hiding this comment

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

@rgsl888prabhu this will be useful in the java side too (we work around this at the moment). One of us can put up a PR after this one is merged to use your new method. Thank you.

CHANGELOG.md Show resolved Hide resolved
cpp/src/unary/null_ops.cu Outdated Show resolved Hide resolved
cpp/src/unary/null_ops.cu Outdated Show resolved Hide resolved
cpp/src/unary/null_ops.cu Outdated Show resolved Hide resolved
Copy link
Collaborator

@kkraus14 kkraus14 left a comment

Choose a reason for hiding this comment

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

Couple of minor Python changes needed

@kkraus14 kkraus14 added 0 - Waiting on Author Waiting for author to respond to review Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer 4 - Needs Review Waiting for reviewer to review or respond labels Oct 17, 2019
@rgsl888prabhu
Copy link
Contributor Author

@kkraus14

@rgsl888prabhu rgsl888prabhu added 3 - Ready for Review Ready for review by team and removed 0 - Waiting on Author Waiting for author to respond to review labels Oct 18, 2019
@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Oct 18, 2019
@shwina shwina merged commit df9917c into rapidsai:branch-0.11 Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] isNotNull and isNull as unary operations on a bitmask vector producing a boolean vector
6 participants