Skip to content

Conversation

@reyoung
Copy link
Collaborator

@reyoung reyoung commented Aug 12, 2017

Fix #3438. The commit is 11c3560.
This PR is based on wangkuiyi#13. Because in wangkuiyi#13, we changed the private members of OperatorBase. Please review wangkuiyi#13 first.

Copy link
Collaborator

@wangkuiyi wangkuiyi left a comment

Choose a reason for hiding this comment

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

LGTM

This is a huge work! Congrats and Thanks!

const std::string Type() const { return type_; }
const std::vector<std::string> Inputs() const { return inputs_; }
const std::vector<std::string> Outputs() const { return outputs_; }
std::string Type() const { return type_; }
Copy link
Member

Choose a reason for hiding this comment

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

这里也应该是

const std::string& Type() const { return type_; }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

可以用另一个PR修改这里。

"Output of FillZerosLikeOp must be set.");
ctx.Output<framework::Tensor>(0)->Resize(
ctx.Input<framework::Tensor>(0)->dims());
ctx.Output<framework::Tensor>("Dst")->Resize(
Copy link
Member

Choose a reason for hiding this comment

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

do not need to check here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这里因为Ouput("")里做了check了。

}

attrs_["temporary_index"] = tmp_index;
auto& inputs = inputs_[kAll];
Copy link
Member

Choose a reason for hiding this comment

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

kALL对应的是map拍平之后的vector么

Copy link
Member

@jacquesqiao jacquesqiao left a comment

Choose a reason for hiding this comment

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

LGTM except for two small questions

: OpProtoAndCheckerMaker(proto, op_checker) {
AddInput("A", "A");
AddInput("B", "B");
AddInput("X", "A");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we need to unify the comment and parameter name? it seems confusing.

for (auto it = forwardNet.ops_.rbegin(); it != forwardNet.ops_.rend();
++it, ++local_op_id) {
auto fwd = *it;
auto bwd = BackwardRecursive(*fwd, no_grad_names, uniq_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

uniq_id use type of reference, which violates the rule in
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

@reyoung reyoung merged commit ffbb0be into PaddlePaddle:develop Aug 14, 2017
@reyoung reyoung deleted the use_ctor_create_op branch October 2, 2017 18:21
heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021
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.

4 participants