[REVIEW] Remove bounds check for `cudf::gather` #6523

isVoid · 2020-10-14T06:52:30Z

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

For out_of_bounds_policy, remove FAIL, and expose NULLIFY and DONT_CHECK to user. NULLIFY sets out of bounds indices to null rows, while DONT_CHECK skips any checking. Using DONT_CHECK should yield higher performance to gather maps with 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 bound checking is added. Will also closes #6731

review-notebook-app · 2020-10-14T06:52:34Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

GPUtester · 2020-10-14T06:52:57Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

cpp/include/cudf/copying.hpp

ajschmidt8 · 2020-10-14T18:03:09Z

Removing ops-codeowners from the required reviews since it doesn't seem there are any file changes that we're responsible for. Feel free to add us back if necessary.

cpp/include/cudf/copying.hpp

codecov · 2020-11-11T05:33:39Z

Codecov Report

Merging #6523 (f90e028) into branch-0.17 (1c81827) will increase coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           branch-0.17    #6523   +/-   ##
============================================
  Coverage        81.94%   81.95%           
============================================
  Files               96       96           
  Lines            16164    16164           
============================================
+ Hits             13246    13247    +1     
+ Misses            2918     2917    -1

Impacted Files	Coverage Δ
python/cudf/cudf/core/frame.py	`90.06% <0.00%> (+0.12%)`	⬆️

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 1c81827...68b2673. Read the comment docs.

isVoid · 2020-11-12T20:55:35Z

rerun tests

python/cudf/cudf/_lib/copying.pyx

jrhemstad

It seems like there are a lot of places where the original behavior has been changed from DONT_CHECK to NULLIFY.

cpp/include/cudf/copying.hpp

jrhemstad · 2020-11-13T13:42:32Z

cpp/src/dictionary/decode.cu

@@ -44,7 +44,7 @@ std::unique_ptr<column> decode(dictionary_column_view const& source,
  // use gather to create the output column -- use ignore_out_of_bounds=true
  auto table_column = cudf::detail::gather(table_view{{source.keys()}},
                                           indices,
-                                           cudf::detail::out_of_bounds_policy::IGNORE,
+                                           cudf::out_of_bounds_policy::NULLIFY,


This seems like it would change the original behavior.

@davidwendt and this?

jrhemstad · 2020-11-13T13:42:43Z

cpp/src/dictionary/remove_keys.cu

@@ -112,7 +112,7 @@ std::unique_ptr<column> remove_keys_fn(
  // Example: gather([0,max,1,max,2],[4,0,3,1,2,2,2,4,0]) => [2,0,max,max,1,1,1,2,0]
  auto table_indices = cudf::detail::gather(table_view{{map_indices->view()}},
                                            indices_view,
-                                            cudf::detail::out_of_bounds_policy::NULLIFY,
+                                            cudf::out_of_bounds_policy::DONT_CHECK,


jrhemstad · 2020-11-13T13:43:15Z

cpp/src/groupby/sort/group_argmax.cu

@@ -51,8 +51,8 @@ std::unique_ptr<column> group_argmax(column_view const& values,
  auto result_table =
    cudf::detail::gather(table_view({key_sort_order}),
                         null_removed_indices,
-                         indices->nullable() ? cudf::detail::out_of_bounds_policy::IGNORE
-                                             : cudf::detail::out_of_bounds_policy::NULLIFY,
+                         indices->nullable() ? cudf::out_of_bounds_policy::NULLIFY


jrhemstad · 2020-11-13T13:43:22Z

cpp/src/groupby/sort/group_argmin.cu

@@ -51,8 +51,8 @@ std::unique_ptr<column> group_argmin(column_view const& values,
  auto result_table =
    cudf::detail::gather(table_view({key_sort_order}),
                         null_removed_indices,
-                         indices->nullable() ? cudf::detail::out_of_bounds_policy::IGNORE
-                                             : cudf::detail::out_of_bounds_policy::NULLIFY,
+                         indices->nullable() ? cudf::out_of_bounds_policy::NULLIFY


jrhemstad · 2020-11-13T13:43:31Z

cpp/src/groupby/sort/groupby.cu

@@ -201,8 +201,8 @@ void store_result_functor::operator()<aggregation::MIN>(aggregation const& agg)
      auto transformed_result =
        cudf::detail::gather(table_view({values}),
                             null_removed_map,
-                             argmin_result.nullable() ? cudf::detail::out_of_bounds_policy::IGNORE
-                                                      : cudf::detail::out_of_bounds_policy::NULLIFY,
+                             argmin_result.nullable() ? cudf::out_of_bounds_policy::DONT_CHECK


jrhemstad · 2020-11-13T13:43:36Z

cpp/src/groupby/sort/groupby.cu

@@ -238,8 +238,8 @@ void store_result_functor::operator()<aggregation::MAX>(aggregation const& agg)
      auto transformed_result =
        cudf::detail::gather(table_view({values}),
                             null_removed_map,
-                             argmax_result.nullable() ? cudf::detail::out_of_bounds_policy::IGNORE
-                                                      : cudf::detail::out_of_bounds_policy::NULLIFY,
+                             argmax_result.nullable() ? cudf::out_of_bounds_policy::DONT_CHECK


cpp/src/transform/encode.cu

jrhemstad · 2020-11-13T13:47:49Z

Can you also update the detail::gather to use out_of_bounds_policy instead of the raw bool here: https://github.com/rapidsai/cudf/blob/branch-0.17/cpp/include/cudf/detail/gather.cuh#L622

We should strive to not have any raw bools in APIs.

isVoid · 2020-11-13T17:33:05Z

It seems like there are a lot of places where the original behavior has been changed from DONT_CHECK to NULLIFY.

I think the replacements result from a change to this line:

cudf/cpp/src/copying/gather.cu

Line 70 in fb789bf

    
           source_table, map_begin, map_end, bounds == out_of_bounds_policy::IGNORE, mr, stream);

Currently when calling cudf::gather with NULLIFY actually gets redirected to IGNORE code path, and this PR includes a fix to it. Accordingly, all calls to gather should be flipped to maintain code semantics (currently only partially replaced).

Addressing your second review comment, this line will be updated without using booleans.

harrism

Just suggesting doc improvements. Note "bounds" is plural in "out of bounds" and "bounds checking".

CHANGELOG.md

cpp/include/cudf/copying.hpp

cpp/include/cudf/detail/gather.hpp

isVoid · 2020-11-16T22:06:16Z

Offline discussion with @jrhemstad : Instead of changing expect_equal to expect_equivalent and force the test cases pass, let's investigate each gather API call and make sure the semantics remain unchanged before and after the refactor.

- Style - Error message Co-authored-by: Ashwin Srinath <[email protected]>

- Use DONT_CHECK in encode.cu

- Undo test_single_agg column test func - style

Ternary in place of if-else block Co-authored-by: nvdbaranec <[email protected]>

- State DONT_CHECK purpose - Style

isVoid · 2020-12-02T00:02:13Z

Closing and defer to another PR for rebase.

isVoid requested review from a team as code owners October 14, 2020 06:52

isVoid requested review from vuule, nvdbaranec, shwina and kkraus14 October 14, 2020 06:52

isVoid changed the base branch from branch-0.16 to branch-0.17 October 14, 2020 06:52

jrhemstad requested changes Oct 14, 2020

View reviewed changes

cpp/include/cudf/copying.hpp Outdated Show resolved Hide resolved

ajschmidt8 removed the request for review from a team October 14, 2020 18:02

jrhemstad reviewed Oct 23, 2020

View reviewed changes

cpp/include/cudf/copying.hpp Outdated Show resolved Hide resolved

jrhemstad reviewed Oct 23, 2020

View reviewed changes

cpp/include/cudf/copying.hpp Outdated Show resolved Hide resolved

isVoid changed the title ~~[WIP] Remove bound check for cudf::gather~~ [REVIEW] Remove bound check for cudf::gather Nov 11, 2020

shwina reviewed Nov 12, 2020

View reviewed changes

python/cudf/cudf/_lib/copying.pyx Outdated Show resolved Hide resolved

shwina reviewed Nov 12, 2020

View reviewed changes

python/cudf/cudf/_lib/copying.pyx Outdated Show resolved Hide resolved

jrhemstad requested changes Nov 13, 2020

View reviewed changes

harrism changed the title ~~[REVIEW] Remove bound check for cudf::gather~~ [REVIEW] Remove bounds check for cudf::gather Nov 16, 2020

harrism requested changes Nov 16, 2020

View reviewed changes

isVoid force-pushed the i6478 branch from 8fbec68 to 0d21339 Compare November 16, 2020 18:38

Michael Wang and others added 25 commits December 1, 2020 13:13

Minmax binding, cython gather bound check

0154590

style

90b93ad

cpp style

bfced20

detail::gather doc update

c25a43c

remove stale couts

6df2dca

Apply suggestions from code review

dca064b

- Style - Error message Co-authored-by: Ashwin Srinath <[email protected]>

style and better error msg

f69d83e

Address review: Refactor detail::gather API

0ba5fd7

Apply doc review offline; style

3ad55a9

Applying Docs Suggestions By Mark

ee1f23b

Resolving bad merge that led to failed compilation

f449087

Address review:

813346a

- Use DONT_CHECK in encode.cu

Review: change out_of_bounds default policy

6c1794a

Partially undoing EQUAL switch; one test failing

f4e3cc4

stale print

bd3f037

Multiple changes:

bbb20e1

- Undo test_single_agg column test func - style

Undoing more EQUIVALENT uses

51e1498

Adapt to DeviceScalar

ed30234

Confirms using NULLIFY with david

acace97

Address review comments: minmax comments

1b7ddaa

Move ternary op outside of function call

2748553

Unsetting nullmasks on gather result

e4cc4b6

style

f1a44dc

Update gather.cuh to address review comments

b67e37a

Ternary in place of if-else block Co-authored-by: nvdbaranec <[email protected]>

Address review comments

68b2673

- State DONT_CHECK purpose - Style

isVoid force-pushed the i6478 branch from f71d4de to 68b2673 Compare December 1, 2020 23:54

isVoid closed this Dec 2, 2020

isVoid mentioned this pull request Dec 2, 2020

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

Merged

shwina mentioned this pull request Feb 16, 2021

[FEA] Add new parameter to cudf::gather to enable behavior of negative indices beget null elements #6479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Remove bounds check for `cudf::gather` #6523

[REVIEW] Remove bounds check for `cudf::gather` #6523

isVoid commented Oct 14, 2020 •

edited by harrism

Loading

review-notebook-app bot commented Oct 14, 2020

GPUtester commented Oct 14, 2020

ajschmidt8 commented Oct 14, 2020

codecov bot commented Nov 11, 2020 •

edited

Loading

isVoid commented Nov 12, 2020

jrhemstad left a comment

jrhemstad Nov 13, 2020

isVoid Nov 18, 2020

jrhemstad Nov 13, 2020

jrhemstad Nov 13, 2020

jrhemstad Nov 13, 2020

jrhemstad Nov 13, 2020

jrhemstad Nov 13, 2020

jrhemstad commented Nov 13, 2020 •

edited

Loading

isVoid commented Nov 13, 2020

harrism left a comment

isVoid commented Nov 16, 2020

isVoid commented Dec 2, 2020

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

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

Conversation

isVoid commented Oct 14, 2020 • edited by harrism Loading

review-notebook-app bot commented Oct 14, 2020

GPUtester commented Oct 14, 2020

ajschmidt8 commented Oct 14, 2020

codecov bot commented Nov 11, 2020 • edited Loading

Codecov Report

isVoid commented Nov 12, 2020

jrhemstad left a comment

Choose a reason for hiding this comment

jrhemstad Nov 13, 2020

Choose a reason for hiding this comment

isVoid Nov 18, 2020

Choose a reason for hiding this comment

jrhemstad Nov 13, 2020

Choose a reason for hiding this comment

jrhemstad Nov 13, 2020

Choose a reason for hiding this comment

jrhemstad Nov 13, 2020

Choose a reason for hiding this comment

jrhemstad Nov 13, 2020

Choose a reason for hiding this comment

jrhemstad Nov 13, 2020

Choose a reason for hiding this comment

jrhemstad commented Nov 13, 2020 • edited Loading

isVoid commented Nov 13, 2020

harrism left a comment

Choose a reason for hiding this comment

isVoid commented Nov 16, 2020

isVoid commented Dec 2, 2020

[REVIEW] Remove bounds check for `cudf::gather` #6523

[REVIEW] Remove bounds check for `cudf::gather` #6523

isVoid commented Oct 14, 2020 •

edited by harrism

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

jrhemstad commented Nov 13, 2020 •

edited

Loading