Skip to content

"add name convention url" #4277

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

Merged
merged 2 commits into from
Sep 21, 2017
Merged

Conversation

dzhwinter
Copy link
Contributor

@dzhwinter dzhwinter commented Sep 21, 2017

添加了命名规范文档。

另外,new_op_cn.md中OpProtoMaker的示例没有遵守命名规范。

class MulOpMaker : public framework::OpProtoAndCheckerMaker {
 public:
  MulOpMaker(framework::OpProto *proto, framework::OpAttrChecker *op_checker)
      : OpProtoAndCheckerMaker(proto, op_checker) {
    AddInput("X", "The first input of mul op");
    AddInput("Y", "The second input of mul op");
    AddOutput("Out", "The output of mul op");
    AddComment(R"DOC(
Two Element Mul Operator.
The equation is: Out = X * Y
)DOC");
  }
};

其中参数注释有点太粗糙了,没有解释清楚参数。例如"The first input of mul op"没有解释任何事情, 改成 "(Tensor)2D tensor of size (M x K)"会好一些。

Input/Output的注释请尽量包括

  1. 参数可以接受的类型
  2. 参数的形状
  3. 参数的约束,含义解释等等

命名规范中对comments的约束是

  • Comments.
    Input/Output/Attr comment follow the format of (type, default value) usage, corresponding to which type it can be and how it will be used by the operator. e.g. Attribute in Accumulator"gamma",(float, default 1.0) Accumulation multiplier.
    Operator comment format of R"DOC(your comment here)DOC". You should explain the input/output of the operator first. If there is math calculation in this operator, you should write the equation in the comment. e.g. Out = X + Y.

命名规范的示例

AddAttr<float>("gamma", "(float, default 1.0) Accumulation multiplier").SetDefault(1.0f);

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++

@dzhwinter dzhwinter merged commit 686f3b8 into PaddlePaddle:develop Sep 21, 2017
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