Add row conv operator #6013
Add row conv operator #6013
Conversation
There was a problem hiding this comment.
@sidgoyal78 I like your code, including the code of the previous PRs. In my opinion, your code is very beautiful and high quality.
paddle/operators/row_conv_op.cc
Outdated
|
|
||
| $$ | ||
| out_{i, :} = \sum_{j=i}^{i + context} in_{j,:} \dot W_{i-j, :} | ||
| $$ |
There was a problem hiding this comment.
For the doc, there are some comments in
https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/function/RowConvOp.cpp#L97
There was a problem hiding this comment.
Thanks for the pointer, i have included now.
paddle/operators/row_conv_op.cc
Outdated
| AddInput("Filter", | ||
| "(Tensor), the input(Filter) is a learnable parameter. It " | ||
| "is a 2-D tensor with shape (future_context x N), where, " | ||
| "future_context is the batch size and N is the data dimension."); |
There was a problem hiding this comment.
I like your code, I think the name future_context is good :)
future_context is the batch size
future_context is the future context length.
paddle/operators/row_conv_op.cc
Outdated
| auto *Out = context.Output<LoDTensor>("Out"); | ||
|
|
||
| Out->mutable_data<T>(context.GetPlace()); | ||
| context.ShareLoD("X", "Out"); |
There was a problem hiding this comment.
Since there is ctx->ShareLoD("X", "Out") in the InferShape, and the previous bug for ShareLoD in InferShape has been fixed, line 123 can be removed.
| cot_seq(k, d) = weights(w, d) * cip_seq(k + w, d); | ||
| } else { | ||
| cot_seq(k, d) += weights(w, d) * cip_seq(k + w, d); | ||
| } |
There was a problem hiding this comment.
Maybe we can use elementwise mul and col-wise sum to remove the for loop in line 145 and line 147. But the optimization can be done in the future. So in this PR, I think it is ok here.
paddle/operators/row_conv_op.cc
Outdated
| void Compute(const framework::ExecutionContext &context) const override { | ||
| auto *X = context.Input<LoDTensor>("X"); | ||
| auto *Filter = context.Input<Tensor>("Filter"); | ||
| auto *Out = context.Output<LoDTensor>("Out"); |
There was a problem hiding this comment.
The naming style:
X->x
Filer->filter
Out->out
https://google.github.io/styleguide/cppguide.html#Variable_Names
There was a problem hiding this comment.
(I need to fix for .cu code)
Resolves #5612 , by adding the implementation of the row-convolution operator.
Few notes: