Replace `map_along_rows` with `matrixVectorOp` #911

Nyrio · 2022-10-10T17:27:11Z

The prim matrixVectorOp is more optimized than the ANN util function map_along_rows. However, in order to substitute the latter, I needed to add better support for differing matrix and vector types.

This PR adds support to arbitrary matrix and vector types in matrix-vector ops. The input and output matrices must have the same type but the vectors can each have different types.

…ulate

…-rows

…d API for matrixVectorOp

…lation

…ecOpTest + remove redundant inputs in LinewiseTest

…rix type + cleanup old matrixVectorOp implementation

…-rows

cjnolet

Changes look good from my end. Just fairly minor things as usual.

cpp/include/raft/linalg/matrix_vector_op.cuh

cjnolet · 2022-10-27T02:47:38Z

cpp/include/raft/linalg/detail/matrix_vector_op.cuh

@@ -22,80 +22,10 @@ namespace raft {
 namespace linalg {
 namespace detail {

-namespace {


Are any of the changes in this file going to break users downstream (such as cuml)?

It won't break cuML because all template types are inferred in calls to matrixVectorOp.
It could in theory break other projects if they provide the template list explicitly, but I'm not aware of any.

cpp/include/raft/linalg/matrix_vector_op.cuh

cpp/include/raft/util/itertools.hpp

…-rows

…lso upstream)

…plementation)

Nyrio · 2022-10-28T18:02:55Z

I've solved the conflicts with #725 and #937 and done the changes requested by @cjnolet

Note that I see failures in the linewise tests with padded spans, not only on this branch but also on branch-22.12. I have informed @mfoerste4 about those failures (I suspect missing boundary checks when reading the vectors).

mfoerste4 · 2022-10-28T21:11:27Z

I've solved the conflicts with #725 and #937 and done the changes requested by @cjnolet

Note that I see failures in the linewise tests with padded spans, not only on this branch but also on branch-22.12. I have informed @mfoerste4 about those failures (I suspect missing boundary checks when reading the vectors).

@Nyrio , thanks for finding this. I added #964 to fix the issue in the test setup.

@Nyrio

This should fix the failures @Nyrio found in [#911](#911 (comment)). This is a test issue within a new testcase that was introduced by [#725](#725). @cjnolet , FYI. Authors: - Malte Förster (https://github.com/mfoerste4) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #964

cjnolet · 2022-10-28T22:42:31Z

@Nyrio the fix to linewise op has been merged.

cjnolet · 2022-10-29T02:00:32Z

rerun tests

Nyrio · 2022-11-02T10:44:20Z

rerun tests

tfeher

Thanks @Nyrio for the PR! It looks good to me.

tfeher · 2022-11-07T07:41:37Z

cpp/include/raft/matrix/detail/linewise_op.cuh


 namespace raft {
 namespace matrix {
 namespace detail {

+/** This type simplifies returning arrays and passing them as arguments */
+template <typename Type, int VecElems>
+struct VecArg {


@Nyrio could you add an issue about this, so that we don't forget it?

cpp/test/linalg/matrix_vector_op.cu

cpp/include/raft/util/itertools.hpp

…-rows

Nyrio · 2022-11-08T18:17:53Z

@tfeher The last commit is for you. Re-requesting a review, so you can have a quick look at the new int8_t/float test.

cjnolet · 2022-11-08T22:11:42Z

@gpucibot merge

Nyrio added 4 commits October 7, 2022 18:35

Integrate accumulate_into_selected into raft prims

7f11225

Remove accumulate_into_selected

529931d

Merge remote-tracking branch 'origin/branch-22.12' into enh-ann-accum…

0f284b0

…ulate

Replace map_along_rows with matrixVectorOp

6a61537

github-actions bot added CMake cpp labels Oct 10, 2022

Nyrio added 2 - In Progress Currenty a work in progress CMake improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed CMake labels Oct 10, 2022

Nyrio mentioned this pull request Oct 13, 2022

Integrate accumulate_into_selected from ANN utils into linalg::reduce_rows_by_keys #909

Merged

Nyrio added 14 commits October 17, 2022 16:08

Merge remote-tracking branch 'origin/branch-22.12' into enh-map-along…

e184ebf

…-rows

Start adding support for arbitrary types in linewiseOp

0a98482

Allow different types for output, matrix and vector(s) in mdspan-base…

eafa61e

…d API for matrixVectorOp

Call cub histogram with signed type to avoid a warning breaking compi…

74309ed

…lation

Start adding support for different output/matrix/vector types in MatV…

df7cfb5

…ecOpTest + remove redundant inputs in LinewiseTest

Fix shared mem buffering offset in linewise kernels

8264462

Pass custom op to naiveMat

e4a8b66

Support different output/matrix/vector(s) types in naiveMatVec

938b180

Test matrix-vector op with different matrix / vector types

c85ee9b

Fix linewiseOp VecRows kernels

be142c6

Fix linewiseOp VecCols kernels

a850d85

Merge OutT=MatT because linewiseOp only supports one input/output mat…

e44bcc4

…rix type + cleanup old matrixVectorOp implementation

Merge remote-tracking branch 'origin/branch-22.12' into enh-map-along…

351fddc

…-rows

Replace for_each with linalg::add + fix syntax error

dcfd962

github-actions bot removed the CMake label Oct 20, 2022

Nyrio marked this pull request as ready for review October 20, 2022 14:44

Nyrio requested a review from a team as a code owner October 20, 2022 14:44

Clang-format fix

4718e5b

cjnolet requested changes Oct 27, 2022

View reviewed changes

Nyrio added 5 commits October 28, 2022 13:18

Merge remote-tracking branch 'origin/branch-22.12' into enh-map-along…

b5c4d01

…-rows

Apply changes to new padded kernel (note: test is still failing but a…

f2f67db

…lso upstream)

Put itertools in util namespace

1450bae

Remove TPB from public API (it wasn't even forwarded to the actual im…

7bcbbe2

…plementation)

Fix utils -> util

c7942b0

mfoerste4 mentioned this pull request Oct 28, 2022

[Hotfix] linewiseop padded span test #964

Merged

cjnolet assigned Nyrio Nov 1, 2022

tfeher approved these changes Nov 7, 2022

View reviewed changes

cjnolet reviewed Nov 7, 2022

View reviewed changes

cpp/include/raft/util/itertools.hpp Outdated Show resolved Hide resolved

Nyrio added 2 commits November 8, 2022 18:19

Merge remote-tracking branch 'origin/branch-22.12' into enh-map-along…

d28a3c5

…-rows

Move product auxiliary function to itertools::detail

68fd977

Nyrio requested a review from cjnolet November 8, 2022 17:45

Add test case for int8_t matrix with float vectors

b2be0c1

Nyrio requested a review from tfeher November 8, 2022 18:16

Nyrio assigned cjnolet and tfeher and unassigned Nyrio Nov 8, 2022

cjnolet approved these changes Nov 8, 2022

View reviewed changes

Nyrio unassigned cjnolet Nov 8, 2022

rapids-bot bot merged commit 1923c87 into rapidsai:branch-22.12 Nov 8, 2022

Nyrio mentioned this pull request Jan 18, 2023

[ENH] IVF-* ANN post-integration TODOs #711

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `map_along_rows` with `matrixVectorOp` #911

Replace `map_along_rows` with `matrixVectorOp` #911

Nyrio commented Oct 10, 2022 •

edited

Loading

cjnolet left a comment

cjnolet Oct 27, 2022

Nyrio Oct 28, 2022 •

edited

Loading

Nyrio commented Oct 28, 2022

mfoerste4 commented Oct 28, 2022

cjnolet commented Oct 28, 2022

cjnolet commented Oct 29, 2022

Nyrio commented Nov 2, 2022

tfeher left a comment

tfeher Nov 7, 2022

Nyrio commented Nov 8, 2022

cjnolet commented Nov 8, 2022

Replace map_along_rows with matrixVectorOp #911

Replace map_along_rows with matrixVectorOp #911

Conversation

Nyrio commented Oct 10, 2022 • edited Loading

cjnolet left a comment

Choose a reason for hiding this comment

cjnolet Oct 27, 2022

Choose a reason for hiding this comment

Nyrio Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Nyrio commented Oct 28, 2022

mfoerste4 commented Oct 28, 2022

cjnolet commented Oct 28, 2022

cjnolet commented Oct 29, 2022

Nyrio commented Nov 2, 2022

tfeher left a comment

Choose a reason for hiding this comment

tfeher Nov 7, 2022

Choose a reason for hiding this comment

Nyrio commented Nov 8, 2022

cjnolet commented Nov 8, 2022

Replace `map_along_rows` with `matrixVectorOp` #911

Replace `map_along_rows` with `matrixVectorOp` #911

Nyrio commented Oct 10, 2022 •

edited

Loading

Nyrio Oct 28, 2022 •

edited

Loading