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

"op name" #3452

Merged
merged 8 commits into from
Sep 13, 2017
Merged

"op name" #3452

merged 8 commits into from
Sep 13, 2017

Conversation

dzhwinter
Copy link
Contributor

@dzhwinter dzhwinter commented Aug 14, 2017

这个PR中只包含了OpProtoMaker应该如何书写,OpProtoMaker中的Input/Output/Attr都会被写入OpProto。在客户端语言创建Op,使用Op的时候会依赖于参数命名规则。在review OpPorting的实践过程中遇到一些问题,更新到该文档中,形成统一的Op参数命名规范。
fix op porting name wiki
#3976
#3899


Variable name is uppercase. e.g. `X`, `Y`

Tensor name is lowercase. e.g. `tensor`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Operator's inputs/outputs are Variable not Tensor.
So what's the meaning of Tensor name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean the variable is a Tensor. fix it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need distinguish input/output's ProtoName and DescName.

Copy link
Member

Choose a reason for hiding this comment

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

if matrix and Out conflict?


### Input/Output names

* Variable name is prefer uppercase. e.g. `X`, `Y`. But when the variable is tensor, its name should lowercase. e.g. `matrix`, to discriminate with otherone.
Copy link
Member

Choose a reason for hiding this comment

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

if the name has multiple letters, such as all, what should we name it All or ALL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We follow the CamelCase but the first character is uppercase. Such as All, Matrix.


* Variable name is prefer uppercase. e.g. `X`, `Y`. But when the variable is tensor, its name should lowercase. e.g. `matrix`, to discriminate with otherone.

* element wise operator, math operator or similar op, please obey common name convention. if the operator only have one output, use `Out`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we shall distinguish between input/outputs' ProtoName and DescName.

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.

@lcy-seso
Copy link
Contributor

lcy-seso commented Sep 8, 2017

我建议我们应该尽快完成并 merge 这个PR。

现在 Op 的命名不仅仅是可读性的问题,也会影响到运行,特别是影响了一些组合 Op 的编写,如果命名不统一,组合 Op 的编写会出现 if-else 分枝来处理特殊命名。

从我们代码目前的设计上看,对命名是敏感的,设计发生变化对现阶段来说是合理的,但,在开发中出现了一些和命名相关的问题,希望能够尽努力有一个标准。是否可以保证 PR merge 之后,大家可以共同维护修改。

从版本库中仅有的例子看,会出现编译通过,但运行时创建 OP 与注册 OP 命名不统一引起的运行时 core dump。以及各种因为忽视命名引发的运行时错误,并且目前的错误信息非常晦涩。

  1. 哪些命名必须强制固定;
  2. 哪些命名应该遵循可读性原则;

这个边界在哪里,对写 OP 的同学应该有一个相对清晰的标准。

Copy link
Contributor Author

@dzhwinter dzhwinter left a comment

Choose a reason for hiding this comment

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

Done.


### Input/Output names

* Variable name is prefer uppercase. e.g. `X`, `Y`. But when the variable is tensor, its name should lowercase. e.g. `matrix`, to discriminate with otherone.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We follow the CamelCase but the first character is uppercase. Such as All, Matrix.

- names follow the `CamelCase` but the first character is uppercase. e.g. `X`, `Y`, `Matrix`, `LastAxisInMatrix`. Input/Output much more like Variables, we prefer to meaningful English words.
- If an operator's Input/Output are not meaningful words, input name starts from `X`. e.g. `X`, `Y`, and output name starts from `Out`. e.g. `Out`.

* Attribute.
Copy link
Collaborator

Choose a reason for hiding this comment

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

有的地方用 * 有的用 - 。太随便了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the format error. I will try my best to write the better document in the future.
fixed.

@@ -0,0 +1,27 @@
## Operator Name Convention
Copy link
Collaborator

Choose a reason for hiding this comment

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

Operator's Parameter Name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix Done.


To make the operator document itself more clear, we recommend operator names obey the listing conventions.

### OpMaker names
Copy link
Collaborator

Choose a reason for hiding this comment

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

OpMaker or OpProtoMaker ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OpProtoMaker. Done.


### OpMaker names

When defining an operator in Paddle, a corresponding `OpMaker` need to be defined. All the `Input`/`Output` and `attrs` will write into the `OpProto` , and will be used in client language to create operator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里需要一个连接来解释什么是OpMaker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的,需要一个文档。暂时Paddle中还没有,添加了TODO.

When defining an operator in Paddle, a corresponding `OpMaker` need to be defined. All the `Input`/`Output` and `attrs` will write into the `OpProto` , and will be used in client language to create operator.

- Input/Output.
- names follow the `CamelCase` but the first character is uppercase. e.g. `X`, `Y`, `Matrix`, `LastAxisInMatrix`. Input/Output much more like Variables, we prefer to meaningful English words.
Copy link
Collaborator

Choose a reason for hiding this comment

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

names => 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.

When defining an operator in Paddle, a corresponding `OpMaker` need to be defined. All the `Input`/`Output` and `attrs` will write into the `OpProto` , and will be used in client language to create operator.

- Input/Output.
- names follow the `CamelCase` but the first character is uppercase. e.g. `X`, `Y`, `Matrix`, `LastAxisInMatrix`. Input/Output much more like Variables, we prefer to meaningful English words.
Copy link
Collaborator

Choose a reason for hiding this comment

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

CamelCase 的第一个字母就是大写的吧。第一个字母小写的叫camelCase。

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.

When defining an operator in Paddle, a corresponding `OpMaker` need to be defined. All the `Input`/`Output` and `attrs` will write into the `OpProto` , and will be used in client language to create operator.

- Input/Output.
- names follow the `CamelCase` but the first character is uppercase. e.g. `X`, `Y`, `Matrix`, `LastAxisInMatrix`. Input/Output much more like Variables, we prefer to meaningful English words.
Copy link
Collaborator

Choose a reason for hiding this comment

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

一对 是表示 code 的,不是用来表示着重号的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@wangkuiyi wangkuiyi merged commit 57d8afe into PaddlePaddle:develop Sep 13, 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.

None yet

6 participants