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

[WIP] Backward #2949

Merged
merged 34 commits into from
Jul 24, 2017
Merged

[WIP] Backward #2949

merged 34 commits into from
Jul 24, 2017

Conversation

dzhwinter
Copy link
Contributor

@dzhwinter dzhwinter commented Jul 18, 2017

naive implement of backward op. Need @Canpio add reorder of input index.

limitations under the License. */

#include "paddle/framework/fully_connected_op.h"
#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

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

 #include <iostream>

#include "paddle/framework/fully_connected_op.h"

@JiayiFeng JiayiFeng changed the title Backward [WIP] Backward Jul 18, 2017
We put forward Op's inputs, outputs and output gradients into Grad
Op's inputs, and put forward Op's input gradients into Grad Op's output.
So Grad Op's `in_out_idx`, `input_format` and 'output format' need to be
rebuilt during Op creating.
*/
#define REGISTER_GRADIENT_OP(__op_type, __op_class) \
STATIC_ASSERT_GLOBAL_NAMESPACE( \
__reg_op__##__op_type, \
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个名字可以再特殊一些,例如

__reg_gradient_op__##__op_type,

否则比较容易和其他的REGISTER冲突。

Copy link
Collaborator

Choose a reason for hiding this comment

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

done

__reg_op__##__op_type, \
"REGISTER_GRADIENT_OP must be in global namespace"); \
static ::paddle::framework::GradOpRegisterHelper<__op_class> \
__op_register_##__op_type##__(#__op_type); \
Copy link
Collaborator

Choose a reason for hiding this comment

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

同理,这个名字也可以特殊一些。。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -274,6 +281,18 @@ class OpRegistry {
return CreateOp(op_desc.type(), inputs, outputs, attrs);
}

static OperatorPtr CreateGradOp(OperatorPtr op) {
OperatorPtr grad_op(grad_creators().at(op->type_)());
Copy link
Collaborator

Choose a reason for hiding this comment

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

If not found that Gradient Op,just return nullptr is cool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -101,5 +101,7 @@ class PlainNet : public Net {
}
};

std::shared_ptr<PlainNet> AddBackwardOp(std::shared_ptr<PlainNet> ForwardOps);
Copy link
Collaborator

@reyoung reyoung Jul 19, 2017

Choose a reason for hiding this comment

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

I think this method might be useless, we can directly invoke OpRegistry::CreateGradOp(...) even that Op is a net.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. I remembered that AddBackwardOp is an interface for PlainNet, which generate the backward ops, then it can be used in Python. Remove it straightly is fine to me.

@JiayiFeng
Copy link
Collaborator

@reyoung reminded us that not all inputs/outputs are required by Op's gradient calculating. So we are going to refactor related functions to support flexible Grad Op inputs/outputs generating.

@@ -274,6 +285,24 @@ class OpRegistry {
return CreateOp(op_desc.type(), inputs, outputs, attrs);
}

static OperatorPtr CreateGradOp(OperatorPtr op) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CreateGradientOp ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we named every Grad operator in XXXGrad, so we create interface named in CreateGradOp

static std::unordered_map<std::string, OpCreator>& creators() {
static std::unordered_map<std::string, OpCreator> creators_;
return creators_;
static void AssembleGradInOut(OperatorPtr op, OperatorPtr grad_op) {
Copy link
Contributor

Choose a reason for hiding this comment

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

AssembleInOutGradient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -63,6 +63,11 @@ class OperatorBase {
/// but it will be convert to a unique name in scope after OpCreator.
static std::string TMP_VAR_NAME() { return "@TEMP@"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

a static const member better? no need a getter

Copy link
Contributor Author

@dzhwinter dzhwinter Jul 20, 2017

Choose a reason for hiding this comment

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

const static member can not ensure the calling order. See the effective C++ rule.

@JiayiFeng JiayiFeng self-requested a review July 24, 2017 11:27
Copy link
Collaborator

@JiayiFeng JiayiFeng left a comment

Choose a reason for hiding this comment

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

LGTM

@JiayiFeng JiayiFeng merged commit 02cde24 into PaddlePaddle:develop Jul 24, 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