Skip to content

Conversation

@luotao1
Copy link
Contributor

@luotao1 luotao1 commented Sep 29, 2017

solve #4319

@luotao1 luotao1 requested a review from Xreki October 9, 2017 04:05
@luotao1
Copy link
Contributor Author

luotao1 commented Oct 9, 2017

The construction of interp_grad op is :

operator.cc:142] Op(plain_net), inputs:{}, outputs:{}.
operator.cc:142] Op(plain_net), inputs:{}, outputs:{}.
operator.cc:142] Op(elementwise_add_grad), inputs:{Out[Out], Out@GRAD[Out@GRAD], X[MulOut], Y[Y]}, outputs:{X@GRAD[MulOut@GRAD], Y@GRAD[Y@GRAD]}.
operator.cc:142] Op(plain_net), inputs:{}, outputs:{}.
operator.cc:142] Op(elementwise_mul_grad), inputs:{Out[MulOut], Out@GRAD[MulOut@GRAD], X[MinusOut], Y[W]}, outputs:{X@GRAD[MinusOut@GRAD], Y@GRAD[W@GRAD]}.
operator.cc:142] Op(plain_net), inputs:{}, outputs:{}.
operator.cc:142] Op(elementwise_sub_grad), inputs:{Out[MinusOut], Out@GRAD[MinusOut@GRAD], X[X], Y[Y]}, outputs:{X@GRAD[X@GRAD], Y@GRAD[Y@GRAD]}.

Copy link
Contributor

@Xreki Xreki left a comment

Choose a reason for hiding this comment

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

Good job! It is almost ok with some naming and description issues for me. About the op's name, I am not sure whether the abbreviation InterpOp is ambiguous.

// MinusOut = X - Y
auto x = Input("X");
auto y = Input("Y");
auto minus_out = Output("MinusOut");
Copy link
Contributor

Choose a reason for hiding this comment

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

How about rename MinusOut to SubOut, since minus op is not used here anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"saving the mul mul of (X - Y) and W")
.AsIntermediate();
AddOutput("Out",
"A 2-D Tensor, the output of interp_op, same shape with X");
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there more detail comments about inputs and outputs? About the format, you can refer to https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/name_convention.md#opprotomaker-names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

About the op's name, I am not sure whether the abbreviation InterpOp is ambiguous.

Discussed with @qingqing01 , as numpy uses numpy.interp, we can use abbreviation name as well.

// MinusOut = X - Y
auto x = Input("X");
auto y = Input("Y");
auto minus_out = Output("MinusOut");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"saving the mul mul of (X - Y) and W")
.AsIntermediate();
AddOutput("Out",
"A 2-D Tensor, the output of interp_op, same shape with X");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@Xreki Xreki left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1 luotao1 merged commit ceefb55 into PaddlePaddle:develop Oct 10, 2017
@luotao1 luotao1 deleted the interp branch October 10, 2017 02:29
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.

2 participants