Add compile time infershape #4569

jacquesqiao · 2017-10-03T02:17:32Z

… add_compile_time_infershape

Superjomn · 2017-10-05T22:57:07Z

paddle/framework/block_desc.cc

@@ -34,6 +34,11 @@ VarDescBind *BlockDescBind::Var(const std::string &name) const {
  return it->second.get();
 }

+bool BlockDescBind::HasVar(const std::string &name) const {
+  auto it = vars_.find(name);


return vars_.count(name) != 0;

return vars.count(name); is more simple : )

find is better to describe the semantic of has.

Since a map can only have at most one key, count will essentially stop after one element has been found. However, in view of more general containers such as multimaps and multisets, find is strictly better if you only care whether some element with this key exists, since it can really stop once the first matching element has been found.

For map, the implement of count is just:

size_type count(const key_type& __x) const { return _M_t.find(__x) == _M_t.end() ? 0 : 1; }

https://stackoverflow.com/questions/25490357/checking-for-existence-in-stdmap-count-vs-find

I see. find could be more efficient than count. So here maybe return vars_.find(name) != vars_end();?

yes, have change to this

Superjomn · 2017-10-05T22:59:42Z

paddle/framework/operator.h

+
+  bool HasInput(const std::string& name) const override {
+    const std::vector<std::string>& input_names = op_.Input(name);
+    PADDLE_ENFORCE_EQ(input_names.size(), 1UL, "Inputs(%s) length is not 1",


the

Inputs(%s) length is not 1

lacks information, tell why should be 1

I second @Superjom that this error message could be more informative. Why must the input length be 1? What is the "input" here?

Superjomn · 2017-10-05T23:00:08Z

paddle/framework/operator.h

+
+  bool HasOutput(const std::string& name) const override {
+    const std::vector<std::string>& output_names = op_.Output(name);
+    PADDLE_ENFORCE_EQ(output_names.size(), 1UL, "Outputs(%s) length is not 1",


same as comment above

Superjomn · 2017-10-05T23:01:41Z

paddle/framework/operator.h

+
+  DDim GetInputDim(const std::string& name) const override {
+    std::vector<DDim> ddims = GetInputsDim(name);
+    PADDLE_ENFORCE_EQ(ddims.size(), 1UL, "Inputs(%s) length is not 1", name);


xxx is not 1 -> xxx should be 1 so that xxx xxx.

Superjomn · 2017-10-05T23:02:00Z

paddle/framework/operator.h

+
+  DDim GetOutputDim(const std::string& name) const override {
+    std::vector<DDim> ddims = GetOutputsDim(name);
+    PADDLE_ENFORCE_EQ(ddims.size(), 1UL, "Outputs(%s) length is not 1", name);


same as above

JiayiFeng · 2017-10-06T00:35:43Z

paddle/framework/block_desc.cc

@@ -34,6 +34,11 @@ VarDescBind *BlockDescBind::Var(const std::string &name) const {
  return it->second.get();
 }

+bool BlockDescBind::HasVar(const std::string &name) const {
+  auto it = vars_.find(name);


return vars.count(name); is more simple : )

JiayiFeng · 2017-10-06T00:46:23Z

paddle/framework/operator.h

+    return block_.HasVar(output_names[0]);
+  }
+
+  bool HasInputs(const std::string& name) const override {


HasInputs() seems to be a more general case of HasInput().Why not Remove HasInput() and only leave this one?

Most of the Time, One input will only have one value,

X = [x1]

HasInput has a check that for X, there should be and only be one value.

for HasInputs, it just requires the length is not zero because the length is not certain.

I don't understand what does the example X=[x1] mean?

Also, what's the difference between HasInput and HasInputs? From the English language, it seems that they would have the same functionality.

In our current implementation, all inputs outputs are repeated filed in protobuf, so the real data sturcture for input/output is:

VarName = [v1, v2, v3 ...]

Most of the time, One VarName only have one corresponding variable, but in some case, the input length is not certain, for example: mul_op will have data like:

X = [x1, x2, x3, ...] Y = [y1, y2, y3, ...] Out = [x1+y1, x2+y2, x3+y3, ...]

But for most Operator, the just need to get the first variable with VarName, so currently we add a helper function

def input(varname): # check varname only have one variable ···

JiayiFeng · 2017-10-06T00:54:31Z

paddle/framework/operator.h

+    return true;
+  }
+
+  DDim GetInputDim(const std::string& name) const override {


GetInputDim & GetInputsDim, it's sooo easy to misuse them for the almost same names. If it's really essential to retain all of them, please give GetInputsDim a more discriminating name. Such as GetMultInputDim.

I would suggest that we merge all Inputs/Input methods into one. For example, HasInput and HasInputs could be one (pseudo code only):

bool HasInput(name) { if (FindVarDesc(name).duplicable == true) { CHECK_LT(0, vars_[name].size()); } else { CHECK_EQ(1, vars_[name].size()); } }

In our current design, the VarDesc did not have the flag duplicable, so we can not use it now~ can optimize the whole interface in next PR~

… add_compile_time_infershape

wangkuiyi · 2017-10-06T17:41:59Z

paddle/framework/block_desc.cc

@@ -34,6 +34,11 @@ VarDescBind *BlockDescBind::Var(const std::string &name) const {
  return it->second.get();
 }

+bool BlockDescBind::HasVar(const std::string &name) const {
+  auto it = vars_.find(name);


I see. find could be more efficient than count. So here maybe return vars_.find(name) != vars_end();?

wangkuiyi · 2017-10-06T17:44:40Z

paddle/framework/operator.h

@@ -317,26 +318,108 @@ class ExecutionContext : public InferShapeContext {
  const platform::DeviceContext& device_context_;
 };

+class CompileTimeInferShapeContext : public InferShapeContextBase {


How about add a TODO comment here:

// TODO(longfei): Once after both CompileTimeInferShapeContext and // RuntimeInferShapeContext get merged, we can rename InferShapeContextBase into // InferShapeContext so to replace the current InferShapeContext.

done, added to shape_inference.h

wangkuiyi · 2017-10-06T17:46:18Z

paddle/framework/operator.h

+
+  bool HasInput(const std::string& name) const override {
+    const std::vector<std::string>& input_names = op_.Input(name);
+    PADDLE_ENFORCE_EQ(input_names.size(), 1UL, "Inputs(%s) length is not 1",


I second @Superjom that this error message could be more informative. Why must the input length be 1? What is the "input" here?

wangkuiyi · 2017-10-06T17:47:18Z

paddle/framework/operator.h

+    return block_.HasVar(output_names[0]);
+  }
+
+  bool HasInputs(const std::string& name) const override {


I don't understand what does the example X=[x1] mean?

wangkuiyi · 2017-10-06T17:48:36Z

paddle/framework/operator.h

+    return block_.HasVar(output_names[0]);
+  }
+
+  bool HasInputs(const std::string& name) const override {


Also, what's the difference between HasInput and HasInputs? From the English language, it seems that they would have the same functionality.

wangkuiyi · 2017-10-06T18:07:41Z

An additional suggestion -- move InferShapeContext, InferShapeContextBase, CompileTimeInferShapeContext out from operator.h into infershape.h. Otherwise, I am afraid that operator.h would be too lengthy.

jacquesqiao · 2017-10-06T19:12:46Z

An additional suggestion -- move InferShapeContext, InferShapeContextBase, CompileTimeInferShapeContext out from operator.h into infershape.h. Otherwise, I am afraid that operator.h would be too lengthy.

Yes, this is one thing need todo after the interface is done.

jacquesqiao · 2017-10-06T20:00:30Z

Have a discussion with @wangkuiyi.

In current Interface, Input and Inputs is used to distinguish input/output that has only one value or has multiple values. It's confusing, so we decided that there should only be one interface

std::vector<Type> Input(const std::string& name);

use should know what they want to use, if they know that input X have only one value, he can do like:

Type x = Input("X")[0];

to get what he want.

I think this should be done in next pr to modify the intreface define and all the related operators.

jacquesqiao added 8 commits October 2, 2017 14:03

tmp

31bdb3f

add CompileTimeInferShapeContext

d550380

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

e0c8cd8

… add_compile_time_infershape

fix compile problem

455436e

optimize infershape context

81fc777

add shape_inference_map

ab9545a

tmp work

5917e09

add python unit test

352af96

jacquesqiao mentioned this pull request Oct 5, 2017

Implement Compile time & Runtime Infershape #4183

Closed

8 tasks

clean code

628715d

jacquesqiao changed the title ~~[WIP]Add compile time infershape~~ Add compile time infershape Oct 5, 2017

clean code

e043b38

Superjomn reviewed Oct 5, 2017

View reviewed changes

JiayiFeng reviewed Oct 6, 2017

View reviewed changes

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

ffe1b69

… add_compile_time_infershape

wangkuiyi reviewed Oct 6, 2017

View reviewed changes

optimize code

8c7ee7a

update comment for input/output length check

4acd5ab

wangkuiyi approved these changes Oct 6, 2017

View reviewed changes

wangkuiyi merged commit f8b5d54 into PaddlePaddle:develop Oct 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compile time infershape #4569

Add compile time infershape #4569

jacquesqiao commented Oct 3, 2017 •

edited

Superjomn Oct 5, 2017

JiayiFeng Oct 6, 2017

jacquesqiao Oct 6, 2017 •

edited

wangkuiyi Oct 6, 2017

jacquesqiao Oct 6, 2017

jacquesqiao Oct 6, 2017

Superjomn Oct 5, 2017

wangkuiyi Oct 6, 2017

jacquesqiao Oct 6, 2017

Superjomn Oct 5, 2017

Superjomn Oct 5, 2017

Superjomn Oct 5, 2017

JiayiFeng Oct 6, 2017

JiayiFeng Oct 6, 2017

jacquesqiao Oct 6, 2017 •

edited

wangkuiyi Oct 6, 2017

wangkuiyi Oct 6, 2017

jacquesqiao Oct 6, 2017 •

edited

JiayiFeng Oct 6, 2017

wangkuiyi Oct 6, 2017

jacquesqiao Oct 6, 2017

wangkuiyi Oct 6, 2017

wangkuiyi Oct 6, 2017

jacquesqiao Oct 6, 2017

jacquesqiao Oct 6, 2017

wangkuiyi Oct 6, 2017

wangkuiyi Oct 6, 2017

wangkuiyi Oct 6, 2017

wangkuiyi commented Oct 6, 2017

jacquesqiao commented Oct 6, 2017

jacquesqiao commented Oct 6, 2017 •

edited

Add compile time infershape #4569

Add compile time infershape #4569

Conversation

jacquesqiao commented Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquesqiao Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquesqiao Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquesqiao Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi commented Oct 6, 2017

jacquesqiao commented Oct 6, 2017

jacquesqiao commented Oct 6, 2017 • edited

jacquesqiao commented Oct 3, 2017 •

edited

jacquesqiao Oct 6, 2017 •

edited

jacquesqiao Oct 6, 2017 •

edited

jacquesqiao Oct 6, 2017 •

edited

jacquesqiao commented Oct 6, 2017 •

edited