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输出的名字? #3922

Closed
Xreki opened this issue Sep 6, 2017 · 5 comments
Closed

是否需要统一Op输出的名字? #3922

Xreki opened this issue Sep 6, 2017 · 5 comments

Comments

@Xreki
Copy link
Contributor

Xreki commented Sep 6, 2017

目前,有的Op使用的输出的名字为Out,比如:

    ScaleOp: Out = scale * X
    IdentityOp: Out = X
    MulOp: Out = X * Y
    AddOp: Out = X + Y 
    MinusOp: Out = X - Y
    ...

有的Op使用的输出的名字为Y,比如:

    SigmoidOp: Y = 1 / (1 - exp(-X))
    SoftmaxOp: Y[i, j] = exp(X[i, j]) / sum_j(exp(X[i, j]))
    OnehotCrossEntropyOp: Y[i] = -log(X[i][j])
    ...

命名的不统一,使得一些组合Op写起来需要特别小心。比如FCOp中需要调用激活函数Op,支持identity, sigmoid, softmax类型,现在代码必须写成:

    auto activation = GetAttr<std::string>("activation");
    if (activation == "identity") {
        AppendOp(framework::OpRegistry::CreateOp(
                 activation, {{"X", {Output("add_out")}}}, {{"Out", {Output("Out")}}}, {}));
    } else {
        AppendOp(framework::OpRegistry::CreateOp(
                 activation, {{"X", {Output("add_out")}}}, {{"Y", {Output("Out")}}}, {}));
    }

鉴于有些Op会使用Y作为输入,Op的输出的名字是否需要统一为Out

@qingqing01
Copy link
Contributor

qingqing01 commented Sep 6, 2017

ProtoMaker定义命名规范:

  • 输入输出命名尽可能更有含义:首字母大写
  • 属性命名也尽可能更有含义:首字母小写
  • 如果没有含义,而且只有一个输出,可以用Out命名: 例如cosine Op: inputs : X, attrs: axis, outputs : Out

这是之前的一些命名约定,对于 SigmoidOp、SoftmaxOp、OnehotCrossEntropyOp输出命名觉得可以统一为Out.

@lcy-seso
Copy link
Contributor

lcy-seso commented Sep 6, 2017

我今天在这个 PR https://github.com/PaddlePaddle/Paddle/pull/3928/files 中遇到了一个和 @Xreki 类似的问题,希望和大家讨论。

这里来描述一下我遇到的问题:

  1. 首先摘抄我们的命名规范:
  • ProtoMaker定义命名规范
    • 输入输出命名尽可能更有含义:首字母大写
    • 属性命名也尽可能更有含义:首字母小写
  • 如果没有含义,而且只有一个输出,可以用Out命名: 例如cosine Op: inputs : X, attrs: axis, outputs : Out
  • @qingqing01 也确认过,Op 的输入尽量起有意义的名字,并非一定需要起名叫做 X。并且,我们的文档里也没有写必须起名叫作 XY
  1. softmax 的输入论文里面一般叫作 logits,于是我想试着将 softmax 的输入 X 改成 Logits,按照上面文档中的命名规范以及 @qingqing01 在此issue 之前的回复,将输出命名为 Out,修改见此 PR https://github.com/PaddlePaddle/Paddle/pull/3928/files

  2. 重命名后 mnist.py 单测挂在此行:https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/framework/tests/mnist.py#L118

    activation_op = Operator(act, X=pre_activation, Y=name)
  • 这一段代码构造激活operator,调用 Operator 构造函数时,遇到softmax激活,第一个输入 X 已经被重命名为 Logits,输出重命名为 Out。但是 sigmoid 激活的第一个输入依然叫做X,输出叫作 Y
  • 上面这一段代码的写法显示地依赖于 Operator 注册时输入输出的名字。
  • 如果我们还想保留这样的写法,就必须统一 Operator 输入输出的名字。
  • 或者从 Proto 中获取输入输出的名字?但我尚为研究应该如何做?

请问:

  • 我们对单输入单输出的激活Operator,应该统一使用 X Y 的命名方式?还是应该修改这个单测?
  • 是否应该统一 Operator 的输入输出命名?

  1. 既然都是单输入单输出的 operator,我试着不使用 key-word argument 方式构造,出现下面的错误提示,截取部分输出信息:
5:   File "mnist.py", line 185, in <module>
145:     fc1 = fc_layer(net=forward_net, input=images, size=100, act="sigmoid")
145:   File "mnist.py", line 121, in fc_layer
145:     activation_op = Operator(act, pre_activation, name)
145:   File "/home/caoying/paddle_codes/paddle_github/build/python/build/lib-python/paddle/v2/framework/op.py", line 161, in __call__
145:     raise ValueError("All Paddle argument should be key-word "
145: ValueError: All Paddle argument should be key-word argument except type

代码出自这里:

https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/framework/op.py#L161

if len(args) != 1:
        raise ValueError("All Paddle argument should be key-word "
                         "argument except type")

我们的出错提示信息有语法错误 ,并且如果是一句完整的话,还是应该加上句号,对吗?

@lcy-seso lcy-seso changed the title 是否需要统一Op输出的名字? 是否需要统一Op输出的名字? Sep 6, 2017
@jacquesqiao
Copy link
Member

jacquesqiao commented Sep 6, 2017


image

如果protomaker中的input output 那么改名了,那么这里create op的时候,也应该对应的修改成一样的名字,比如X要改成 Logits,Y改成Out

变成

activation_op = Operator(act, Logits=pre_activation, Out=name)

现在的问题是不希望手动来修改这个地方么?

@lcy-seso
Copy link
Contributor

lcy-seso commented Sep 6, 2017

  • 可以注意看一下 minst.py 中 fc_layer 的实现。
  • 现在实现的几个激活(sigmoid, softmax (如果重命名输出输入),identity(如果也算激活))输入输出名字不统一。
  • fc_layer 这样的写法,没办法用通用的方式来支持定义激活。而且违背了我们在命名规范中提到的:”使用尽量有意义的名字。“

@Xreki
Copy link
Contributor Author

Xreki commented Sep 13, 2017

Op参数命名规则,已经在#3996 中给出了初步的定案。故关闭此issue。

@Xreki Xreki closed this as completed Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants