Skip to content

Comments

CPU Batch Norm Op#4964

Merged
jacquesqiao merged 28 commits intoPaddlePaddle:developfrom
jacquesqiao:batch-norm-op
Oct 24, 2017
Merged

CPU Batch Norm Op#4964
jacquesqiao merged 28 commits intoPaddlePaddle:developfrom
jacquesqiao:batch-norm-op

Conversation

@jacquesqiao
Copy link
Member

@jacquesqiao jacquesqiao commented Oct 20, 2017

Background

project: #4531
fix: #4906

Progress

  • CPU forward
  • CPU gradient
  • Support NCHW && NHWC
  • Python Unit Test(forward && gradient)

@jacquesqiao jacquesqiao changed the title Batch norm op [WIP] Batch Norm Op Oct 20, 2017
@jacquesqiao jacquesqiao self-assigned this Oct 20, 2017
@jacquesqiao jacquesqiao requested a review from zchen0211 October 21, 2017 22:15
@jacquesqiao jacquesqiao changed the title [WIP] Batch Norm Op CPU Batch Norm Op Oct 21, 2017
Copy link
Contributor

@zchen0211 zchen0211 left a comment

Choose a reason for hiding this comment

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

Generally LGTM.


PADDLE_ENFORCE(x_dims.size() >= 3 && x_dims.size() <= 5,
"The Input dim size should be between 3 and 5");
const int N = x_dims[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to enforce Google coding style?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

saved_mean_e.setZero();
saved_variance_e.setZero();

switch (tensor_format) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A complete implementation, which is very nice. Do we only need to do NCHW or NHWC? We can discuss.

Copy link
Member Author

Choose a reason for hiding this comment

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

I find our old code support both format, and Tensorflow/Caffe2 also support two data format, so I think maybe we need them all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our old code only support NCHW (for 2D Conv) or NCDHW (for 3D Conv), not support NHWC and NDHWC. And all the convolution operators in new framework only support NCHW(for 2D Conv) orNCDHW` too.

Copy link
Member Author

Choose a reason for hiding this comment

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

👌

@zhouxiao-coder
Copy link
Contributor

Do we need to support 2D input data? For example, if the previous layer is a fully connected layer.

enum TensorFormat {
NHWC = 0,
NCHW = 1,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

There is DataLayout in paddle/platform/cudnn_helper.h.

Copy link
Member Author

Choose a reason for hiding this comment

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

cudnn_helper cannot be included in a cc file, I think maybe we need to move it to another place

? (tensor_format == TensorFormat::NCHW ? x_dims[4] : x_dims[3])
: 1;

const int sample_size = H * W * D;
Copy link
Contributor

Choose a reason for hiding this comment

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

输入是5D: 即 NCDHWNDHWC:
可以去掉line 136 - line 144复杂的获取方法,line 146改成:

const int frame_size =  x->numel() / N / C;

saved_mean_e.setZero();
saved_variance_e.setZero();

switch (tensor_format) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Our old code only support NCHW (for 2D Conv) or NCDHW (for 3D Conv), not support NHWC and NDHWC. And all the convolution operators in new framework only support NCHW(for 2D Conv) orNCDHW` too.


switch (tensor_format) {
case TensorFormat::NCHW: {
ConstEigenArrayMap<T> X_arr(x->data<T>(), sample_size, N * C);
Copy link
Contributor

Choose a reason for hiding this comment

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

saved_mean_e /= N * sample_size;
for (int nc = 0; nc < N * C; ++nc) {
saved_variance_e(nc % C) +=
(X_arr.col(nc) - saved_variance_e(nc % C))
Copy link
Contributor

Choose a reason for hiding this comment

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

saved_variance_e -> saved_mean_e?

saved_variance_e(nc % C) +=
    (X_arr.col(nc) - saved_mean_e(nc % C))

? (tensor_format == TensorFormat::NCHW ? x_dims[4] : x_dims[3])
: 1;

const int sample_size = H * W * D;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above.

// init output
auto *dX = ctx.Output<Tensor>(framework::GradVarName("X"));
auto *dScale = ctx.Output<Tensor>(framework::GradVarName("Scale"));
auto *dBias = ctx.Output<Tensor>(framework::GradVarName("Bias"));
Copy link
Contributor

Choose a reason for hiding this comment

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

The names of variables (including function parameters) and data members are all lowercase

https://google.github.io/styleguide/cppguide.html#Variable_Names

@@ -0,0 +1,424 @@
/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reserve => Reserved.

Copy link
Member Author

Choose a reason for hiding this comment

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

all the code is wrong now, I will update them in another PR

@@ -0,0 +1,62 @@
/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reserve => Reserved

Copy link
Contributor

@zchen0211 zchen0211 left a comment

Choose a reason for hiding this comment

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

LGTM.

@jacquesqiao jacquesqiao merged commit ee998a9 into PaddlePaddle:develop Oct 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.

impl batch norm operator

5 participants