Op varient inputs #2901

Superjomn · 2017-07-16T07:28:30Z

No description provided.

…ent-inputs

jacquesqiao · 2017-07-16T14:28:01Z

paddle/framework/operator.cc

 namespace paddle {
 namespace framework {

+void OperatorBase::CreateArgumentOffsetMap(const OpProto& proto) {


Argument in caffe2 has the same meaning with Attribute in Paddle. Should we name it CreateInOutOffsetMap or something else?

this map stores both the inputs and outputs offests, so maybe CreateArgmentOffsetsMap ?

InOut instead of Input ~

jacquesqiao · 2017-07-16T14:29:47Z

paddle/framework/operator.h

  virtual void Run(const ScopePtr& scope,
                   const platform::DeviceContext& dev_ctx) const = 0;

+  // Get a input with argument's name described in `op_desc`


described in op_proto or op_proto_and_checker_maker

jacquesqiao · 2017-07-16T14:41:03Z

paddle/framework/operator.cc

+
+const std::string& OperatorBase::Input(const std::string& name) const {
+  auto it = arg_idxs_.find(name);
+  PADDLE_ENFORCE(it != arg_idxs_.end(), "no key [%d] in arg_idxs_", name);


[%d] --> [%s]

jacquesqiao · 2017-07-16T14:41:19Z

paddle/framework/operator.cc

+
+const std::string& OperatorBase::Output(const std::string& name) const {
+  auto it = arg_idxs_.find(name);
+  PADDLE_ENFORCE(it != arg_idxs_.end(), "no key [%d] in arg_idxs_", name);


[%d] --> [%s]

jacquesqiao · 2017-07-16T15:14:59Z

paddle/framework/operator_test.cc

+  void Run(const std::shared_ptr<Scope>& scope,
+           const platform::DeviceContext& dev_ctx) const override {
+    float scale = GetAttr<float>("scale");
+    ASSERT_NEAR(scale, 3.14, 1e-5);


can remove the unnecessary assert

qingqing01 · 2017-07-16T15:41:21Z

paddle/framework/operator.cc

+  if (attrs_.count("input_format") == 0) {
+    return inputs_[it->second];
+  } else {
+    const auto& input_format = GetAttr<std::vector<int>>("input_format");


What does the input_format mean? If it is the necessary attribute for dynamic-length inputs, should check it in the op_checker ?

qingqing01 · 2017-07-16T15:50:25Z

paddle/framework/operator_test.cc

+  OpKernelTestMultiInputsProtoAndCheckerMaker(OpProto* proto,
+                                              OpAttrChecker* op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInputs("xs", "input of test op");


inputs of test op

qingqing01 · 2017-07-16T15:51:53Z

paddle/framework/operator_test.cc

+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddInputs("xs", "input of test op");
+    AddInput("k", "input of test op");
+    AddOutputs("ys", "output of test op");


outputs of test op

qingqing01 · 2017-07-16T15:53:30Z

paddle/framework/operator_test.cc

+  attr->set_f(3.14);
+
+  auto attr0 = op_desc.mutable_attrs()->Add();
+  attr0->set_name("input_format");


input_starts or input_offsets?

reference here: https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/op_registry.h#L131

reyoung · 2017-07-17T03:07:30Z

paddle/framework/operator.h

+  void CreateInOutOffsetMap(const OpProto& proto);
+
+ protected:
+  std::string Type() const { return type_; }


type_ is better.

jacquesqiao · 2017-07-17T03:36:45Z

paddle/framework/operator.cc

+void OperatorBase::CreateInOutOffsetMap(const OpProto& proto) {
+  for (int i = 0; i < proto.inputs_size(); i++) {
+    const auto& name = proto.inputs()[i].name();
+    arg_idxs_[name] = i;


in_out_inxs?

actually, index --> indices?

But it doesn't matter, we all understand what arg_idxs_ means.

为了保持一致，建议还是改成 in_out_idxs 之类的。

jacquesqiao · 2017-07-17T04:05:22Z

paddle/framework/operator_test.cc

-    ASSERT_EQ((int)context.op_.outputs_.size(), 1);
-    ASSERT_NEAR(context.op_.GetAttr<float>("scale"), 3.14, 1e-5);
+  void Compute(const KernelContext& ctx) const {
+    std::cout << "this is cpu kernel" << std::endl;


cpu_kernel_run_num++ should be added back

reyoung · 2017-07-17T04:16:11Z

paddle/framework/operator.cc

+  }
+}
+
+std::vector<std::string> OperatorBase::Inputs(const std::string& name) const {


Possible optimization here. Return a vector_view instead of a vector to prevent memory copy. It is not critical for now. But we can still have a TODO comments here.

added in header

done

reyoung · 2017-07-17T04:22:35Z

paddle/framework/enforce.h

 limitations under the License. */

 #pragma once
+#include <glog/logging.h>


Why need this include?

jacquesqiao

LGTM! except for a little question: What is the purpose of move CreateInOutOffsetMap from OperatorBase to op registry

Superjomn added 3 commits July 15, 2017 12:40

add inputs

937b099

add ut for multiple inputs

bec798b

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into op-vari…

5a0233c

…ent-inputs

Superjomn requested review from jacquesqiao, qingqing01 and reyoung July 16, 2017 07:28

Superjomn added 2 commits July 16, 2017 15:29

recover enforce

e58ddea

format code

f0f029d

jacquesqiao reviewed Jul 16, 2017

View reviewed changes

qingqing01 reviewed Jul 16, 2017

View reviewed changes

Superjomn and others added 7 commits July 17, 2017 07:17

Merge branch 'develop' into op-varient-inputs

2c3473a

fix conflicts

771db04

fix AddToLayer

edd0d01

op_desc -> op_proto

84eef6f

review fix�

d57d9bc

CreateArgumentOffsetMap -> CreateInOutOffsetMap

0113c4c

remove NEAR

a8e3c8a

reyoung reviewed Jul 17, 2017

View reviewed changes

Superjomn added 2 commits July 17, 2017 11:11

remove Type()

010a1d8

remote unused scales

5459382

jacquesqiao reviewed Jul 17, 2017

View reviewed changes

reyoung reviewed Jul 17, 2017

View reviewed changes

Superjomn added 2 commits July 17, 2017 12:52

remove glog

39be0ba

add comments

b2d77a2

Superjomn added 2 commits July 17, 2017 14:42

move CreateInOutOffsetMap from OperatorBase to op registry

0327d6c

arg_idxs_ -> in_out_idxs_

d7f5de9

reyoung approved these changes Jul 17, 2017

View reviewed changes

jacquesqiao approved these changes Jul 17, 2017

View reviewed changes

Superjomn merged commit a0caf23 into PaddlePaddle:develop Jul 17, 2017

Superjomn deleted the op-varient-inputs branch July 17, 2017 08:03

Op varient inputs #2901

Op varient inputs #2901

Uh oh!

Conversation

Superjomn commented Jul 16, 2017

Uh oh!

jacquesqiao Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Superjomn Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacquesqiao Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacquesqiao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jacquesqiao Jul 16, 2017 •

edited

Loading

Superjomn Jul 17, 2017 •

edited

Loading

jacquesqiao Jul 17, 2017 •

edited

Loading