Skip to content
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

Using constructor to create an operator. #3444

Merged
merged 2 commits into from
Aug 14, 2017

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

@@ -51,8 +50,8 @@ class MulOpMaker : public OpProtoAndCheckerMaker {
public:
MulOpMaker(OpProto *proto, OpAttrChecker *op_checker)
: 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.

@@ -93,9 +103,11 @@ std::shared_ptr<OperatorBase> BackwardRecursive(
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.

None yet

4 participants