gather function only #3191
gather function only #3191
Conversation
There was a problem hiding this comment.
The first batch of comments is all about coding style. Please read https://google.github.io/styleguide/cppguide.html thoroughly before coding C++.
paddle/operators/gather_func.h
Outdated
| */ | ||
| template <typename place, typename T> | ||
| Tensor* Gather_func(Tensor* Src, Tensor* Index) { | ||
| // assert index is an int-type tensor? |
There was a problem hiding this comment.
Remove unnecessary code instead commenting them out.
paddle/operators/gather_func.h
Outdated
| * input[Index]: type-int index Tensor (1-D) | ||
| * return: output tensor | ||
| */ | ||
| template <typename place, typename T> |
There was a problem hiding this comment.
place ==> Place,
We are following Google C++ style, which requires that type names in Camel case: https://google.github.io/styleguide/cppguide.html#Type_Names
paddle/operators/gather_func.h
Outdated
| // assert(Index->istype(int)); | ||
|
|
||
| // check index of shape 1-D | ||
| assert(Index->dims().size()==1); |
There was a problem hiding this comment.
Please use PADDLE_ENFORE defined at
Paddle/paddle/platform/enforce.h
Line 155 in 6824c09
paddle/operators/gather_func.h
Outdated
| int index_size = Index->dims()[0]; | ||
|
|
||
| // Source shape | ||
| auto src_dims = Src->dims(); |
There was a problem hiding this comment.
The indentation doesn't match Google C++ style. This error implies that you are not using pre-commit to automatically reformat your code.
Please run pip install pre-commit to install pre-commit. Then Paddle repo's configuration will enable Git to call it whenever you run git commit.
paddle/operators/gather_func.h
Outdated
|
|
||
| /* slice size */ | ||
| int slice_size = 1; | ||
| for(unsigned int i = 0; i < src_dims.size(); ++i) |
There was a problem hiding this comment.
unsigned int ==> size_t, which is the return type of src_dims.size().
paddle/operators/gather_func.h
Outdated
| * return: output tensor | ||
| */ | ||
| template <typename place, typename T> | ||
| Tensor* Gather_func(Tensor* Src, Tensor* Index) { |
There was a problem hiding this comment.
Src => src
Index => index
Please follow https://google.github.io/styleguide/cppguide.html#Variable_Names when naming variable.
paddle/operators/gather_func.h
Outdated
| * return: output tensor | ||
| */ | ||
| template <typename place, typename T> | ||
| Tensor* Gather_func(Tensor* Src, Tensor* Index) { |
There was a problem hiding this comment.
Gather_func => Gather
Please follow https://google.github.io/styleguide/cppguide.html#Function_Names when naming functions.
paddle/operators/scatter_func.h
Outdated
|
|
||
| for(int i = 0; i < index_size; ++i) | ||
| int index_ = index[i]; | ||
| /* dst[index_] += src[index_] |
There was a problem hiding this comment.
Remove unnecessary code instead of commenting them out.
paddle/operators/gather_func.h
Outdated
|
|
||
| // Source shape | ||
| auto src_dims = src->dims(); | ||
| DDim output_dims(dims_src); |
paddle/operators/gather_func.h
Outdated
| // Create a tensor of shape [index_size, dim_src[1:]] | ||
| output_dims[0] = index_size; | ||
|
|
||
| Tensor* New_tensor; |
There was a problem hiding this comment.
Tensor* New_tensor = new Tensor(); use shared_ptr maybe better
|
Function looks like this one. |
paddle/operators/gather.h
Outdated
| } | ||
| } | ||
|
|
||
| /* Implementation of GPU copy: |
paddle/operators/gather.h
Outdated
| CPUGather<T>(src->data<T>(), index->data<int>(), slice_size, index_size, | ||
| output->data<T>()); | ||
| } else { | ||
| // init for GPU |
paddle/operators/gather.h
Outdated
| #include "paddle/framework/tensor.h" | ||
| #include "paddle/platform/place.h" | ||
|
|
||
| using paddle::framework::Tensor; |
There was a problem hiding this comment.
According to Google C++ style guide https://google.github.io/styleguide/cppguide.html#Namespaces, no using in header files.
paddle/operators/gather_test.cc
Outdated
| Tensor* src = new Tensor(); | ||
| Tensor* index = new Tensor(); | ||
| Tensor* output = new Tensor(); | ||
| // src.Resize(make_ddim({3, 4})); |
paddle/operators/gather_test.cc
Outdated
| p_index[0] = 1; | ||
| p_index[1] = 0; | ||
|
|
||
| // gather |
paddle/operators/gather.h
Outdated
| */ | ||
| template <typename T> | ||
| void GPUGather(const T* src, const int* index, const int slice_size, | ||
| const int index_size, T* output); |
There was a problem hiding this comment.
Do you plan to implement this function template in a future PR?
| void GPUGather(const T* src, const int* index, const int slice_size, | ||
| const int index_size, T* output); | ||
|
|
||
| /** |
I add the gather functions, with unit-test passed.
The function is very similar to https://www.tensorflow.org/api_docs/python/tf/gather.