Cmrnorm #854

hedaoyuan · 2016-12-13T06:29:01Z

该PR实现的功能

Fix CPU version of crossMapNormalBwd is not implemented #294
增加Paddle Function
两部分合并在一起写是把normal当作一个例子指导后续Matrix API的重构。以后Paddle新增一个算法功能，实现上不再是Matrix增加一个算法API，而是Paddle增加一个算法Function。

Paddle Function的用法见 #892 的描述，对应到该PR，新增cross map normalize Function

定义CrossMapNormal/CrossMapNormalGrad 实现算法API；
将API以FunctionBase的形式，封装成CrossMapNormalFunc/CrossMapNormalGradFunc；
CMRProjectionNormLayer::init里面create forward/backward function；
CMRProjectionNormLayer::forward/backward里面调用相应的function；
在cross_map_normal_op_test.cpp里面增加TEST；

tianbingsz · 2016-12-13T22:47:46Z

paddle/math/cross_map_normal_op.cpp

+
+  const int start = -((int)sizeX) / 2;
+  const int end = (int)sizeX + start;
+  const real ratio = -(real)2 * scale * pow;


can we write: -2.0 * scale * pow ?

tianbingsz · 2016-12-13T22:48:01Z

paddle/math/Matrix.cpp

+
+  const int start = -((int)sizeX) / 2;
+  const int end = (int)sizeX + start;
+  const real ratio = -(real)2 * scale * pow;


can we write: -2.0 * scale * pow ?

I think we cannot since 2.0 is considered a double-typed value by the compiler, but real is a macro which could evaluate to float.

but const real ratio might convert double into float if real is float.

but const real ratio might convert double into float if real is float. There may be warning.

tianbingsz · 2016-12-13T22:50:23Z

paddle/math/cross_map_normal_op.cpp

+}
+
+template <>
+void CrossMapNormalGrad<DEVICE_TYPE_CPU>::operator()(CpuMatrix& inputsGrad,


can you write down the math expression here for better understanding the code?

The backward formula is derived from the forward formula.

tianbingsz · 2016-12-13T22:51:57Z

paddle/math/cross_map_normal_op.cpp

+
+// NCHW
+template <>
+void CrossMapNormal<DEVICE_TYPE_CPU>::operator()(CpuMatrix& outputs,


can you write down the math expression here for better understanding the code?

tianbingsz

Looks very good, I only have minor comments.

hedaoyuan · 2016-12-15T09:39:52Z

paddle/math/tests/test_matrixCompare.cpp

@@ -1262,6 +1263,150 @@ TEST(Matrix, MaxOutFwdBwd) {
  }
 }

+void testCrossMapNormalFwd(


Test暂时先写这里了，后续会移到function里面。

hedaoyuan · 2016-12-15T09:43:17Z

paddle/function/cross_map_normal_op.cpp

+
+REGISTER_TYPED_FUNC(CrossMapNormal, CPU, CrossMapNormalFunc);
+REGISTER_TYPED_FUNC(CrossMapNormalGrad, CPU, CrossMapNormalGradFunc);
+#ifndef PADDLE_ONLY_CPU


后续会把PADDLE_ONLY_CPU移到REGISTER_TYPED_FUNC里面判断

可能有的Function实现会不支持GPU，这个暂时不修改。

hedaoyuan · 2016-12-15T14:41:40Z

paddle/gserver/layers/Layer.h

@@ -100,6 +101,11 @@ class Layer {
  /// Mark input grad in(true) or out(false) of backward function.
  std::vector<bool> markInBackward_;

+  /// Layer forward function
+  FunctionBase* forward_;


后续会修改成std::vector，一个Layer可以包含多个Function。

hedaoyuan · 2016-12-15T14:41:58Z

paddle/gserver/layers/Layer.h

@@ -100,6 +101,11 @@ class Layer {
  /// Mark input grad in(true) or out(false) of backward function.
  std::vector<bool> markInBackward_;

+  /// Layer forward function
+  FunctionBase* forward_;


后续会修改成std::vector，一个Layer可以包含多个Function。

hedaoyuan · 2016-12-15T15:05:38Z

paddle/gserver/layers/NormProjectionLayer.cpp

@@ -45,6 +45,23 @@ bool CMRProjectionNormLayer::init(const LayerMap& layerMap,
  /* the size of inputs for norm-layer is 1 */
  CHECK_EQ(config_.inputs_size(), 1);

+  if (useGpu_) {


后续会写个宏，简化这段代码。

tianbingsz · 2016-12-16T01:57:28Z

Very exciting, will follow the work.

qingqing01 · 2016-12-19T15:21:48Z

paddle/gserver/layers/NormProjectionLayer.cpp

+  forward_->calc(
+      {Tensor(input->getData(), dims_)},
+      {Tensor(outV->getData(), dims_), Tensor(denoms_->getData(), dims_)},
+      {});


后续是否会改为数据构造的时候就是Tensor呢？而不是中间转换呢？

作为Function的参数的数据结构和用于计算的数据结构是两个，中间会有一次转换。
作为参数的数据结构（能表示多维，多类型的数据）是用来替代现在的paddle::Argument，用于计算的数据结构是继承于paddle::TensorExpression。后面会分别写两个issue来解释这两个事情。

tianbingsz

Look very good to me, only minor comments. You may ignore some of them for the fast development.

tianbingsz · 2016-12-20T04:59:52Z

paddle/function/Function.cpp

+
+template <>
+FuncConfig& FuncConfig::set<size_t>(const std::string& key, size_t v) {
+  CHECK(valueMap_.count(key) == 0) << "Duplicated value: " << key;


Should we use count > 0 to indicate there is already a value for the key?
CHECK(valueMap_.count(key) > 0) << "Duplicated value: " << key;

CHECK(true) will pass. There need valueMap_.count(key) == 0 is true.

这里应该是 CHECK_EQ(0, valueMap_.count(key)) ，这样万一count返回的不是0，则 glog 可以打印出count返回的值，以便debug。

CHECK ==> CHECK_EQ

tianbingsz · 2016-12-20T05:00:26Z

paddle/function/Function.cpp

+
+template <>
+FuncConfig& FuncConfig::set<real>(const std::string& key, real v) {
+  CHECK(valueMap_.count(key) == 0) << "Duplicated value: " << key;


same as line 35

same as above

CHECK ==> CHECK_EQ

tianbingsz · 2016-12-20T05:04:12Z

paddle/function/cross_map_normal_op.h

+template <DeviceType Device>
+void CrossMapNormal(real* outputs,
+                    real* denoms,
+                    real* inputs,


Should we use const for inputs, such as const real * denoms? Anyway, if it is too complicated to do, please ignore this comment.

tianbingsz · 2016-12-20T05:05:24Z

paddle/function/cross_map_normal_op.h

+ */
+template <DeviceType Device>
+void CrossMapNormalGrad(real* inputsGrad,
+                        real* inputsValue,


same as line 40

tianbingsz · 2016-12-20T05:06:21Z

paddle/function/cross_map_normal_op_gpu.cu

+  const int idx = threadIdx.x + blockIdx.x * blockDim.x;
+  if (idx < imageSize) {
+    const int w = idx % width;
+    const int h = (idx / width) % height;


Should we check width > 0 and height > 0 here? Seems not so important here though.

size_t is unsigned

unsigned could be 0.

tianbingsz · 2016-12-20T05:07:18Z

paddle/function/cross_map_normal_op_gpu.cu

+template <>
+void CrossMapNormal<DEVICE_TYPE_GPU>(real* outputs,
+                                     real* denoms,
+                                     real* inputs,


Should we write like const real* inputs?

tianbingsz · 2016-12-20T05:08:04Z

paddle/function/cross_map_normal_op_gpu.cu

+
+template <>
+void CrossMapNormalGrad<DEVICE_TYPE_GPU>(real* inputsGrad,
+                                         real* inputsValue,


same as line 66.

wangkuiyi

后知后觉的补充了一些comments。和 #973 里说的是一样的。

wangkuiyi · 2016-12-20T21:42:56Z

paddle/function/Function.cpp

+template <>
+size_t FuncConfig::get<size_t>(const std::string& key) const {
+  auto it = valueMap_.find(key);
+  CHECK(it != valueMap_.end()) << "Cannot find value: '" << key << "'";


CHECK ==> CHECK_NE

CHECK_NE not support type of it.

wangkuiyi · 2016-12-20T21:43:12Z

paddle/function/Function.cpp

+
+template <>
+FuncConfig& FuncConfig::set<size_t>(const std::string& key, size_t v) {
+  CHECK(valueMap_.count(key) == 0) << "Duplicated value: " << key;


CHECK ==> CHECK_EQ

wangkuiyi · 2016-12-20T21:46:26Z

paddle/function/Function.cpp

+
+template <>
+FuncConfig& FuncConfig::set<real>(const std::string& key, real v) {
+  CHECK(valueMap_.count(key) == 0) << "Duplicated value: " << key;


CHECK ==> CHECK_EQ

wangkuiyi · 2016-12-20T21:46:49Z

paddle/function/Function.cpp

+template <>
+real FuncConfig::get<real>(const std::string& key) const {
+  auto it = valueMap_.find(key);
+  CHECK(it != valueMap_.end()) << "Cannot find value: '" << key << "'";


CHECK ==> CHECK_NE

CHECK_NE not support type of it.

wangkuiyi · 2016-12-20T21:48:03Z

paddle/function/FunctionTest.h

+
+}  // namespace paddle
+
+using paddle::FunctionCompare;


header files 不应该有 using directtive，尤其是在 global namespace 里使用using。详情请见 #973

Done, see pr #1006

emailweixu · 2017-01-04T19:43:39Z

paddle/function/Function.h

+
+  virtual void init(const FuncConfig& config) {}
+
+  virtual void calc(const Arguments& inputs,


Need to have detailed comment for this interface function.

* add FewCLUE 9 datasets * fix a bug for tnews * Add CI for Ernie text matching * Add CI for Ernie text matching * Add CI for Ernie text matching * fix encoding problem for windows * update ernie_text_matching ci * standard GPU id for CI * standard GPU id for CI * add simcse * update train.py * support multi-card training * add README.md Co-authored-by: Zeyu Chen <chenzeyu01@baidu.com>

hedaoyuan added 3 commits December 12, 2016 18:17

cpu cmrnorm

529f24c

add CrossMapNormal

9503590

add GPU CrossMapNormal

e357f27

tianbingsz reviewed Dec 13, 2016

View reviewed changes

tianbingsz suggested changes Dec 13, 2016

View reviewed changes

hedaoyuan added 8 commits December 14, 2016 18:46

add Function

a1d2abc

Add a Tensor to use as a Function argument

ce1d98e

imporve Function

4ebb3eb

Merge branch 'develop' of https://github.com/baidu/Paddle into cmrnorm

9171ab0

add CrossMapNormalGradFunc

d2d0010

move Function to function dir

22a5e47

add CMakeLists

558e869

Remove some useless code

d11e2b4

hedaoyuan commented Dec 15, 2016

View reviewed changes

hedaoyuan assigned emailweixu and tianbingsz Dec 15, 2016

hedaoyuan mentioned this pull request Dec 15, 2016

Paddle Function #892

Closed

hedaoyuan added 2 commits December 15, 2016 20:12

fix swig_api

f13aeb5

add some comments

cee9346

hedaoyuan commented Dec 15, 2016

View reviewed changes

add Layer::createFunction

148bd4d

qingqing01 reviewed Dec 19, 2016

View reviewed changes

tianbingsz suggested changes Dec 20, 2016

View reviewed changes

hedaoyuan added 3 commits December 20, 2016 17:20

move TEST from test_matrixCompare.cpp to cross_map_normal_op_test.cpp

5fddd99

Merge branch 'develop' of https://github.com/baidu/Paddle into cmrnorm

bf32411

follow comments

f1a94e3

tianbingsz approved these changes Dec 20, 2016

View reviewed changes

tianbingsz merged commit 42e1217 into PaddlePaddle:develop Dec 20, 2016

wangkuiyi reviewed Dec 20, 2016

View reviewed changes

hedaoyuan mentioned this pull request Dec 23, 2016

follow some comments with issue 973 #1006

Merged

emailweixu reviewed Jan 4, 2017

View reviewed changes


		virtual void init(const FuncConfig& config) {}

		virtual void calc(const Arguments& inputs,

Cmrnorm #854

Cmrnorm #854

Conversation

hedaoyuan commented Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi Dec 13, 2016 • edited Loading

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

tianbingsz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedaoyuan Dec 15, 2016 • edited Loading

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

tianbingsz commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianbingsz left a 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

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 left a 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

hedaoyuan commented Dec 13, 2016 •

edited

Loading

wangkuiyi Dec 13, 2016 •

edited

Loading

hedaoyuan Dec 15, 2016 •

edited

Loading