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 helper function for generating grad var's name #3188

Closed
wants to merge 8 commits into from

Conversation

Superjomn
Copy link
Contributor

No description provided.

namespace op_helpers {

// Generate the gradient variable's name of a forward varialbe
inline std::string GenGradName(const std::string& var) {
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 renaming GRAD_VAR_SUFFIX and making it return var + grad_suffix directly may be a better choice? Introducing a new namespace make things more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good ideas. but there are some strange GRAD_VAR_SUFFIX usages, such as

https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/backward.cc#L138
https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/backward.cc#L150

It seems that GRAD_VAR_SUFFIX is necessary for these usages, GenGradName is added to hide the details of grad var's name from developers.
@Canpio

The namespace op_helpers is added, because operators needs many helper functions, GenGradName is the first, many other ones should be offered, such some helpers to help these simple check:

PADDLE_ENFORCE(ctx.InputSize() == 2,
               "Input size of OnehotCrossEntropyOp must be two");
PADDLE_ENFORCE(ctx.OutputSize() == 1,
               "Output size of OnehotCrossEntropyOp must be one");

may need a helper

void EnforceInputsOutputsCountMatchProto();
PADDLE_ENFORCE(ctx.InputVar(0) != nullptr && ctx.InputVar(1) != nullptr,
               "Inputs of OnehotCrossEntropyOp must all be set");
PADDLE_ENFORCE(ctx.OutputVar(0) != nullptr,
               "Outputs of OnehotCrossEntropyOp must all be set");

may need a helper

void EnforceVarsValid(xxx);
PADDLE_ENFORCE(ctx.Input<Tensor>(0)->dims() == ctx.Input<Tensor>(1)->dims(),
               "Two input of Add Op's dimension must be same.");

may need a helper

void EnforceShapeMatches(const std::vector<std::string>& var_names);

So, I will create an op_helpers directory and keep this namespace.
@wangkuiyi

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Superjom @Canpio

Rename GRAD_VAR_SUFFIX --> GenGradName().

string GenGradName(string name) {
  return name + "@GRAD";
}

and suffix could be GenGradName("");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

work, but GenGradName("") is a little weird, but it can help to hide GRAD_VAR_SUFFIX. I will follow this idea first.
@reyoung

@@ -319,5 +319,14 @@ class OperatorWithKernel : public OperatorBase {
virtual void InferShape(const InferShapeContext& ctx) const = 0;
};

namespace op_helpers {
Copy link
Collaborator

Choose a reason for hiding this comment

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

为什么需要放在 op_helpers 这个namespace里呢?看上去可以在 framework 里就好了。

根据 https://google.github.io/styleguide/cppguide.html#Namespace_Names ,如果要引入 op_helpers,需要创建一个子目录叫 op_helpers。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get it.

@wangkuiyi wangkuiyi mentioned this pull request Aug 2, 2017
* variable is the gradient of another varibale.
* e.g. Variable "x@GRAD" is the gradient of varibale "x".
*/
inline std::string GenGradName(const std::string& var) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

需要为这么一个函数创建一个namespace吗?是很快就会有其他函数被加入进来吗?如果不是,就放到 framework namespace里是不是“最简单”的做法?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

对,应该很快会加op inputs/outputs 一些check的helper function,简化一部分 InferShape 里的写法

@Superjomn Superjomn closed this Aug 4, 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.

4 participants