-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Conversation
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.
LGTM
This is a huge work! Congrats and Thanks!
paddle/framework/operator.h
Outdated
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_; } |
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.
这里也应该是
const std::string& Type() const { return type_; }
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.
可以用另一个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( |
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.
do not need to check here?
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.
这里因为Ouput("")里做了check了。
paddle/operators/net_op.cc
Outdated
} | ||
} | ||
|
||
attrs_["temporary_index"] = tmp_index; | ||
auto& inputs = inputs_[kAll]; |
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.
kALL对应的是map拍平之后的vector么
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.
LGTM except for two small questions
paddle/framework/backward_test.cc
Outdated
@@ -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"); |
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.
maybe we need to unify the comment and parameter name? it seems confusing.
paddle/framework/backward.cc
Outdated
@@ -93,9 +103,11 @@ std::shared_ptr<OperatorBase> BackwardRecursive( | |||
auto fwd = *it; | |||
auto bwd = BackwardRecursive(*fwd, no_grad_names, uniq_id); |
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.
uniq_id use type of reference, which violates the rule in
https://google.github.io/styleguide/cppguide.html#Reference_Arguments
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.