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

Reduce by key #2254

Merged
merged 9 commits into from
Feb 7, 2020
Merged

Reduce by key #2254

merged 9 commits into from
Feb 7, 2020

Conversation

syurkevi
Copy link
Contributor

Implements one dimensional reduce by key for all backends.

include/af/algorithm.h Outdated Show resolved Hide resolved
src/api/c/reduce.cpp Outdated Show resolved Hide resolved
Copy link
Member

@umar456 umar456 left a comment

Choose a reason for hiding this comment

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

I only did a quick run through this PR. I didn't look into the logic just yet. There is a lot of stuff here. There are a couple of suggestions I have made. Your tests are only testing your float code paths. They should be testing all types. There is also a lot of repetition there. I think things can be improved. It is exciting that we will finally have a reduce by key in ArrayFire. Awesome work.

include/af/algorithm.h Outdated Show resolved Hide resolved
include/af/algorithm.h Show resolved Hide resolved
src/api/c/reduce.cpp Outdated Show resolved Hide resolved
src/api/c/reduce.cpp Outdated Show resolved Hide resolved
src/api/c/reduce.cpp Show resolved Hide resolved
test/reduce.cpp Outdated Show resolved Hide resolved
test/reduce.cpp Outdated Show resolved Hide resolved
test/reduce.cpp Outdated Show resolved Hide resolved
test/reduce.cpp Outdated Show resolved Hide resolved
test/reduce.cpp Outdated Show resolved Hide resolved
@syurkevi syurkevi force-pushed the reduce_by_keym branch 2 times, most recently from 95d867e to cafdf69 Compare August 2, 2018 22:36
@9prady9
Copy link
Member

9prady9 commented Sep 18, 2018

@syurkevi Is this ready for review ? Has umar's feedback been addressed ?

@syurkevi
Copy link
Contributor Author

@syurkevi Is this ready for review ? Has umar's feedback been addressed ?

All outstanding feedback has been addressed, however we were also considering handling the multidimensional cases. If the multidimensional case needs to be added then it isn't ready yet.

@mlloreda
Copy link
Member

@syurkevi this doesn't build for me unless I add #include "spdlog/sinks/stdout_sinks.h" to Logger.cpp.

@9prady9
Copy link
Member

9prady9 commented Oct 26, 2018

That's probably so because this branch is quite outdated from master.

test/reduce.cpp Outdated Show resolved Hide resolved
test/reduce.cpp Outdated Show resolved Hide resolved
Copy link
Member

@9prady9 9prady9 left a comment

Choose a reason for hiding this comment

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

Looks good. I need to do another pass on the kernels of each backend.

Copy link
Contributor

@mark-poscablo mark-poscablo left a comment

Choose a reason for hiding this comment

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

forge's commit SHA is older than master's in this PR. Should that be updated?

test/reduce.cpp Outdated Show resolved Hide resolved
@9prady9 9prady9 mentioned this pull request Mar 13, 2019
@WilliamTambellini
Copy link
Contributor

WilliamTambellini commented Jun 13, 2019

Hi @mark-poscablo
Could you tell if that PR would allow to do a "ragged max" :
input
1 2
3 4
5 6
keys
3 2
expected output
5 4

  • the max of the first col is 5 because we are considering the 3 elements of the first column
  • the max of the 2nd col is supposed to be 6 but that max reduction returns 4 because the algo only has to look at the 2 first elements of the 2nd column

Tks
W.

@mark-poscablo
Copy link
Contributor

mark-poscablo commented Jun 13, 2019

Hi @WilliamTambellini , we should probably ask the author @syurkevi, but I think this PR does not address that use case. Reduce by key would do something like the following:

Using maxByKey:

Input : 1 2 3 4 5 6 7 8 9
Keys  : 1 1 1 2 2 2 3 3 3
Output: 3 6 9

It essentially does the reduction operation on groups, where elements that have the same keys are in the same group.

syurkevi and others added 2 commits January 28, 2020 01:48
adds multidimensional tests of varying sizes and types to match one dimensional
cases
@umar456 umar456 force-pushed the reduce_by_keym branch 2 times, most recently from 34ed8ee to c844c72 Compare January 29, 2020 08:16
Copy link
Member

@9prady9 9prady9 left a comment

Choose a reason for hiding this comment

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

I have reviewed api level (c/cpp/unified) source code and CPU backend implementation. I am still going through CUDA backend. Will submit another set of comments once that is done.

@@ -43,11 +43,49 @@ namespace af
AFAPI array sum(const array &in, const int dim, const double nanval);
#endif

#if AF_API_VERSION >= 37
/**
C++ Interface for sum of elements along given dimension by key
Copy link
Member

Choose a reason for hiding this comment

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

Is there an order to the keys that will be present in the output keys for all the newly added *ByKey functions ? We need to mention this somewhere in the documentation whatever the expectation should be.

src/api/c/reduce.cpp Show resolved Hide resolved
src/api/c/reduce.cpp Show resolved Hide resolved
src/api/c/reduce.cpp Show resolved Hide resolved
src/backend/cpu/kernel/reduce.hpp Outdated Show resolved Hide resolved
src/backend/cpu/kernel/reduce.hpp Show resolved Hide resolved
src/backend/cuda/kernel/reduce_by_key.hpp Outdated Show resolved Hide resolved
src/backend/cuda/kernel/reduce_by_key.hpp Show resolved Hide resolved
src/backend/cuda/kernel/reduce_by_key.hpp Outdated Show resolved Hide resolved
src/api/c/reduce.cpp Outdated Show resolved Hide resolved
src/api/c/reduce.cpp Outdated Show resolved Hide resolved
src/backend/cuda/kernel/reduce_by_key.hpp Outdated Show resolved Hide resolved
src/api/c/reduce.cpp Show resolved Hide resolved
src/backend/cuda/kernel/reduce_by_key.hpp Show resolved Hide resolved
src/backend/cuda/kernel/shfl_intrinsics.hpp Show resolved Hide resolved
src/backend/cuda/kernel/reduce_by_key.hpp Outdated Show resolved Hide resolved
src/backend/cuda/kernel/reduce_by_key.hpp Outdated Show resolved Hide resolved
src/backend/cuda/kernel/reduce_by_key.hpp Outdated Show resolved Hide resolved
@umar456 umar456 merged commit 3b7141f into arrayfire:master Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants