Skip to content

Comments

Feature/pybind for protobuf desc#4397

Merged
reyoung merged 45 commits intoPaddlePaddle:developfrom
reyoung:feature/pybind_for_protobuf_desc
Sep 27, 2017
Merged

Feature/pybind for protobuf desc#4397
reyoung merged 45 commits intoPaddlePaddle:developfrom
reyoung:feature/pybind_for_protobuf_desc

Conversation

@reyoung
Copy link
Collaborator

@reyoung reyoung commented Sep 26, 2017

No description provided.

reyoung and others added 30 commits September 21, 2017 20:13

op.set_block_attr("block_attr", prog.block(0))
self.assertEqual(0, op.get_block_attr("block_attr"))
self.assertEqual(core.AttrType.INT, op.attr_type("int_attr"))
Copy link
Member

Choose a reason for hiding this comment

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

this line should be moved to line 26

};

class OpDescBind {
public:
Copy link
Member

Choose a reason for hiding this comment

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

need a space before public/private/protected

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, there are some missing clang-format files. I fix it in PR #4402. However, we should merge this PR before #4402 for resolving conflict easily.

}

void SetAttr(const std::string &name, const Attribute &v) {
this->attrs_[name] = v;
Copy link
Member

Choose a reason for hiding this comment

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

need

need_update_ = true;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


private:
OpDesc op_desc_;
std::unordered_map<std::string, std::vector<std::string>> inputs_;
Copy link
Member

Choose a reason for hiding this comment

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

should add a comment that: every method change these data members should set
need_update_ = true;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

return ops_.back().get();
}

OpDescBind *PrependOp() {
Copy link
Member

Choose a reason for hiding this comment

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

what is the situation of PrependOp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for init_ops

.def(
"new_var", &BlockDescBind::NewVar, py::return_value_policy::reference)
.def("var", &BlockDescBind::Var, py::return_value_policy::reference)
.def("all_vars",
Copy link
Member

Choose a reason for hiding this comment

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

need interface:

has_input()
has_output()
has_var()

for infer_shape

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since when writing this PR, infer_shape has not been merged. So the interface will be added in next PR.

return prog_->Block(static_cast<size_t>(this->desc_->parent_idx()));
}

void OpDescBind::SetBlockAttr(const std::string &name, BlockDescBind &block) {
Copy link
Member

@jacquesqiao jacquesqiao Sep 26, 2017

Choose a reason for hiding this comment

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

我记得之前讨论是把BlockDisk的index存到 attr里边,这里为什么把整个结构都保存下来了,而且Get也只能Get到index

@reyoung reyoung force-pushed the feature/pybind_for_protobuf_desc branch 2 times, most recently from 62a4fda to de35098 Compare September 27, 2017 02:51
Copy link
Member

@jacquesqiao jacquesqiao left a comment

Choose a reason for hiding this comment

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

LGTM!

return it->second;
}

int GetBlockAttr(const std::string &name) const {
Copy link
Member

Choose a reason for hiding this comment

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

还有一个小问题,既然SetBlockAttr是保存了BlockDesc的结构,那GetBlockAttr何不直接返回BlockDesc?拿到BlockDesc也是可以拿到index的

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

因为这个接口要返回给Python呀。。

@reyoung reyoung merged commit dcfd31d into PaddlePaddle:develop Sep 27, 2017
@reyoung reyoung deleted the feature/pybind_for_protobuf_desc branch September 27, 2017 03:26
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.

3 participants