-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Port fully connected operator #3927
Conversation
…tput name of identity to Y.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work except some name conventions.
paddle/operators/fc_op.cc
Outdated
"sum", {{"X", {mul_out}}}, {{"Out", {Output("SumOut")}}}, {})); | ||
} else { | ||
AppendOp(framework::OpRegistry::CreateOp( | ||
"identity", {{"X", {mul_out[0]}}}, {{"Y", {Output("SumOut")}}}, {})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
identity
operator is not needed by fc
. We could just remove SumOut
, like
outputs_["SumOut"].clear();
But it could be done in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried such kind of implementation, "renaming SumOut
to EMPTY
and using mul_out[0]
instead", but so sad to meet some problem. I will try again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works. I think there may be some error in my previous implementation.
paddle/operators/fc_op.cc
Outdated
|
||
auto activation = Attr<std::string>("activation"); | ||
AppendOp(framework::OpRegistry::CreateOp( | ||
activation, {{"X", {Output(add_out)}}}, {{"Y", {Output("Y")}}}, {})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reference https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/name_convention.md
It seems that our Output
prefer to Out
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that. But I prefer to use Y
😞 Whatever, I will rename it.
Thus, the output Out is a 2-D matrix of size (M x N). | ||
Activation type can be set to `identity` (default), `sigmoid` or `softmax`. | ||
)DOC"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
paddle/operators/fc_op.cc
Outdated
USE_OP(rowwise_add); | ||
USE_NO_KERNEL_OP(identity); | ||
USE_OP(sigmoid); | ||
USE_OP(softmax); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
179-183行不用加了,可参考 #4119
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
… inputs and outputs in FCOp.
"Output(Out) of FCOp should not be null."); | ||
|
||
auto x = Inputs("X"); | ||
auto w = Inputs("W"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the following code only use x.size() and w.size()
so here can use
size_t x_size = Inputs("X").size();
size_t w_size = Inputs("W").size();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X
and W
are duplicatable inputs, and x[i]
and w[i]
are used in line 73
which creates mul
operators. w.size()
is only used in the PADDLE_ENFORCE_EQ
statement in line 39
.
resolve #3926
TestFCGradOp
, operator should be defined as:Do not try to call operators with inplace arguments, such as
Y = X + Y
. In my first implementation, I constructFCOp
as a series of operators as follows:mul_out = X[0] * W[0]
add_out = X[1] * W[1]
mul_out = mul_out + add_out
add_out = add_out + b
Y = Act(add_out)
The unittest failed when constructed an
add
operator during the backward process. I found twoadd
operators were automatically created and theDebugString
are:Thus I changed the implementation of
FCOp
to:1.
mul_out[0] = X[0] * W[0]
2.
mul_out[1] = X[1] * W[1]
3.
sum_out = mul_out[0] + mul_out[1]
4.
add_out = sum_out + b
5.
Y = Act(add_out)
std::vector
cannot be set default value, see How to set the default value of a std::vector<int> attribute #4077 - solved.