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] Remove bounds check for cudf::gather #6875

Merged
merged 45 commits into from
Dec 2, 2020
Merged

[REVIEW] Remove bounds check for cudf::gather #6875

merged 45 commits into from
Dec 2, 2020

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Dec 2, 2020 •
edited
Loading

Closes #6478

cudf::gather now will not run a pre-pass to check for index validity.

For out_of_bounds_policy, remove FAIL, while exposing NULLIFY and DONT_CHECK to user. NULLIFY checks out-of-bounds indices and sets them to null rows, while DONT_CHECK skips all checks. Using DONT_CHECK should yield higher performance, given gather_map contains only valid indices.

Note that the negative index (wrap-arounds) policy is unchanged. When gather map dtype is signed, wrap-around is applied.

A new Cython binding to cudf::minmax, used for Cython gather bound checking is added. Will also close #6731

Michael Wang and others added 30 commits December 1, 2020 17:04
Initial
e286317 
- Remove bound check in detail::gather
- Chnage input argument for cudf::gather from bool check_bound to
  out_of_bound_policy
- Expose out_of_bound_policy
- Reduce ob_policy to only Nullify and Ignore
- Change ob_policy usage
changelog
176da20
Wrap around control
0e39bb1
Revert unsigned type gather map change
006a729
Document update, passes build
962efe3
Update docs
eff09da
Doc updates
db19402
Changing to NULLIFY defaults for compatibility
3bbc7a6
Reducing failed test cases
bf78dff 
Replacing `EQUAL` with `EQUIVALENCE` test, `remove_key` gather calls
repaced with `DONT_CHECK` policy
General replacement; redcuce failed cases
2382ee9
@isVoid @jrhemstad
Update cpp/include/cudf/copying.hpp
b606411 
Co-authored-by: Jake Hemstad <[email protected]>
Retaining code logic before change
d709136
Minmax binding, cython gather bound check
527372d
style
39a73a4
cpp style
174350a
detail::gather doc update
446b809
remove stale couts
4b135bb
@isVoid @shwina
Apply suggestions from code review
3216887 
- Style
- Error message

Co-authored-by: Ashwin Srinath <[email protected]>
style and better error msg
dc6f5cd
Address review: Refactor detail::gather API
a244bf5
Apply doc review offline; style
1f4d4e2
@isVoid
Applying Docs Suggestions By Mark
be5e953
Address review:
a401cc2 
- Use DONT_CHECK in encode.cu
Review: change out_of_bounds default policy
5fd81a6
Partially undoing EQUAL switch; one test failing
3188f67
stale print
060719c
Multiple changes:
48fca22 
- Undo test_single_agg column test func
- style
Undoing more EQUIVALENT uses
b0e7c98
Adapt to DeviceScalar
9ee83ca
Confirms using NULLIFY with david
9827d41
devavret
devavret approved these changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

Already approved the previous PR

kkraus14
kkraus14 reviewed Dec 2, 2020
View reviewed changes
python/cudf/cudf/_lib/cpp/reduce.pxd Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/copying.pyx Show resolved Hide resolved
@kkraus14 kkraus14 added the Python Affects Python cuDF API. label Dec 2, 2020
@kkraus14 kkraus14 added 5 - Merge After Dependencies 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 5 - Merge After Dependencies labels Dec 2, 2020
isVoid
isVoid commented Dec 2, 2020
View reviewed changes
cpp/include/cudf/copying.hpp Outdated Show resolved Hide resolved
@harrism harrism changed the title [REVIEW] Remove bounds check for cudf::gather (Rebase) [REVIEW] Remove bounds check for cudf::gather Dec 2, 2020
@harrism
Copy link
Member

harrism commented Dec 2, 2020

@isVoid can you please add a clear description to this PR? Then we can add Okay to automerge.

@kkraus14 kkraus14 added 0 - Waiting on Author Waiting for author to respond to review and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels Dec 2, 2020
@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge 6 - Okay to Auto-Merge and removed 0 - Waiting on Author Waiting for author to respond to review labels Dec 2, 2020
@rapids-bot rapids-bot bot merged commit a2d2726 into rapidsai:branch-0.17 Dec 2, 2020
@jlowe jlowe mentioned this pull request Dec 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

@kkraus14 kkraus14 kkraus14 approved these changes

@shwina shwina shwina approved these changes

@devavret devavret devavret approved these changes

@harrism harrism Awaiting requested review from harrism

Assignees

@isVoid isVoid

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change feature request New feature or request 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] minmax reduction binding in Cython/Python [FEA] Remove bounds checking from cudf::gather
8 participants
@isVoid @GPUtester @harrism @devavret @shwina @kkraus14 @jrhemstad @nvdbaranec