Refactorize Tensor to Eigen convesion #2953

wangkuiyi · 2017-07-18T22:06:28Z

I rewrote the interpretation of Tensor as Eigen::TensorMap. It seems that the amount of source code can be significantly reduce. Also, code can be put together other than scattered around in multiple source files.

QiJune · 2017-07-19T04:38:23Z

paddle/operators/add_op.h

-        input0.flat<T>() + input1.flat<T>();
+    framework::EigenVector<T>::From(*output).device(
+        *(context.GetEigenDevice<Place>())) =
+        framework::EigenVector<T>(*input0) + framework::EigenVector<T>(*input1);


This line should be

framework::EigenTensor<T, rank>::From(*output).device( *(context.GetEigenDevice<Place>())) = framework::EigenTensor<T, rank>::From(input0)+ framework::EigenTensor<T, rank>::From(input1);

But we could not get the template parameter rank. So, we need a flatten method to reshape a tensor with multi-dimension into one dimension.

Sure. I understand that we need Flatten. I just didn't have enough time today to add it. If would be great if you can take this PR and go on finishing it. Thanks!

QiJune · 2017-07-19T04:53:17Z

paddle/framework/eigen_test.cc

+  }
+
+  EigenTensor<float, 3>::Type et = EigenTensor<float, 3>::From(t);
+  // TODO: check the content of et.


The check can be:

for (int i = 0; i < 1 * 2 * 3; i++) { EXPECT_EQ(et(i), static_cast<float>(i)); }

reyoung · 2017-07-19T08:23:26Z

paddle/framework/eigen.h

+template <typename T, size_t D, typename IndexType = Eigen::DenseIndex>
+struct EigenTensor {
+  using Type = Eigen::TensorMap<Eigen::Tensor<T, D, Eigen::RowMajor, IndexType>,
+                                Eigen::Aligned>;


We cannot think all memory is aligned even our memory allocator is aligned because we could invoke Tensor::Slice or change Tensor::offset_.

Yes, and now, make EigenTensor default unaligned.
And we will make a benchmark between aligned and unaligned version in future.

reyoung · 2017-07-19T08:24:15Z

paddle/framework/eigen.h

+};
+
+// Interpret paddle::platform::Tensor as EigenTensor and EigenConstTensor.
+template <typename T, size_t D, typename IndexType = Eigen::DenseIndex>


Also, even our default configuration is RowMajor, but we could also make RowMajor to here.

Done. Just make RowMajor default.

reyoung · 2017-07-19T08:30:46Z

paddle/framework/eigen.h

+
+// Interpret paddle::platform::Tensor as EigenMatrix and EigenConstMatrix.
+template <typename T, typename IndexType = Eigen::DenseIndex>
+struct EigenMatrix {


Why not

template <typename T, typename IndexType = Eigen::DenseIndex> using EigenMatrix = EigenTensor<T, 2, IndexType>;

reyoung · 2017-07-19T08:35:55Z

paddle/framework/eigen.h

+
+// Interpret paddle::platform::Tensor as EigenVecotr and EigenConstVector.
+template <typename T, typename IndexType = Eigen::DenseIndex>
+struct EigenVector {


Why not

template <typename T, typename IndexType = Eigen::DenseIndex> class EigenVector: public EigenTensor<T, 1, IndexType> { Flatten() {...} };

reyoung · 2017-07-19T10:22:52Z

paddle/framework/eigen.h

+template <typename T, size_t D, int MajorType = Eigen::RowMajor,
+          typename IndexType = Eigen::DenseIndex>
+struct EigenTensor {
+  // TODO(qijun) Now, default type in unaligned, and we will make a benchmark on


Good TODO comment!

reyoung

Basically LGTM, but please do not compare float value with == or EQ. That will unit-test failed in some situation. Please change that before merge.

reyoung · 2017-07-19T10:23:58Z

paddle/framework/eigen.h

+  // Flatten is to reshape a Tensor into a one dimension EigenVector
+  static typename EigenTensor<T, 1>::Type Flatten(Tensor& tensor) {
+    return EigenTensor<T, 1>::From(
+        tensor, make_ddim({static_cast<int>(product(tensor.dims_))}));


tensor, {static_cast<int>(product(tensor.dims_)))} is enough. But it does not matter.

reyoung · 2017-07-19T10:24:42Z

paddle/framework/eigen_test.cc

+
+TEST(EigenDim, From) {
+  EigenDim<3>::Type ed = EigenDim<3>::From(make_ddim({1, 2, 3}));
+  EXPECT_EQ(1, ed[0]);


I think we should prefer ASSERT_XXX than EXPECT_XXX, because ASSERT_XXX die soon.

reyoung · 2017-07-19T10:25:13Z

paddle/framework/eigen_test.cc

+
+TEST(Eigen, Tensor) {
+  Tensor t;
+  float* p = t.mutable_data<float>(make_ddim({1, 2, 3}), platform::CPUPlace());


You can use {1, 2, 3} now, make_ddim is a little noising.

reyoung · 2017-07-19T10:26:16Z

paddle/framework/eigen_test.cc

+  for (int i = 0; i < 1; i++) {
+    for (int j = 0; j < 2; j++) {
+      for (int k = 0; k < 3; k++) {
+        EXPECT_EQ((i * 2 + j) * 3 + k, et(i, j, k));


Never use EQ on float type. Use ASSERT_NEAR instead.

reyoung · 2017-07-19T10:26:39Z

paddle/framework/eigen_test.cc

+  EXPECT_EQ(6, ev.dimension(0));
+
+  for (int i = 0; i < 6; i++) {
+    EXPECT_EQ(i, ev(i));


ASSERT_NEAR

reyoung · 2017-07-19T10:32:57Z

paddle/operators/add_op.h

@@ -29,8 +30,10 @@ class AddKernel : public framework::OpKernel {

    output->mutable_data<T>(context.GetPlace());

-    output->flat<T>().device(*(context.GetEigenDevice<Place>())) =
-        input0.flat<T>() + input1.flat<T>();
+    framework::EigenVector<T>::Flatten(*output).device(


Very noising code for implement kernel. Maybe we should think a better way after this PR.

Maybe that way is OK?

struct OpKernelUseEigen : public framework::OpKernel { protected: template <typename T> using Flatten = framework::EigenVector<T>::Flatten; };

wangkuiyi · 2017-07-20T02:15:07Z

paddle/framework/eigen.h

+struct EigenVector : public EigenTensor<T, 1, MajorType, IndexType> {
+  // Flatten is to reshape a Tensor into a one dimension EigenVector
+  static typename EigenTensor<T, 1>::Type Flatten(Tensor& tensor) {
+    return EigenTensor<T, 1>::From(


Flatten的return typeEigenTensor<T,1>实际上是 EigenTensor<T, 1, Eigen::RowMajor, Eigen::DenseIndex>。而EigenVector被specialized的时候，可能用户传进来的template parameters不是 RowMajor和DenseIndex。结果就可能是用户写：

auto et = EigenVector<float, Eigen::ColumnMajor, Eigen:SparseIndex>::Flatten(t);

的时候，心里期待 et 的类型是 EigenTensor<float, 1, ColumnMajor, SparseIndex>，但是实际上是 EigenTensor<float, 1, RowMajor, DenseIndex>了?

对，这里bug了。。

struct EigenVector : public EigenTensor<T, 1, MajorType, IndexType> { Type Flatten(...); };

应该就可以。

@wangkuiyi Yes, Got it! I am fixing it now.

Refactorize Tensor to Eigen convesion

642d3c4

wangkuiyi requested review from QiJune, reyoung, jacquesqiao, gangliao and JiayiFeng July 18, 2017 22:06

Yi Wang added 4 commits July 18, 2017 15:35

Add unit test

cb1d1f1

Update

00ed564

Fix wrong inclusion path

2538e20

Fix Tensor::data interface

1981eaf

QiJune reviewed Jul 19, 2017

View reviewed changes

QiJune added 2 commits July 19, 2017 14:06

add Flatten method to EigenVector

d9fa615

add more uinttest for EigenTensor

cff8762

reyoung reviewed Jul 19, 2017

View reviewed changes

make EigenTensor default unaligned and follow comments

57c27b4

reyoung reviewed Jul 19, 2017

View reviewed changes

reyoung approved these changes Jul 19, 2017

View reviewed changes

change EQ to NEAR for float value

d6d057b

QiJune merged commit d810849 into PaddlePaddle:develop Jul 19, 2017

wangkuiyi commented Jul 20, 2017

View reviewed changes

wangkuiyi deleted the tensor_type_to_eigen branch July 21, 2017 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactorize Tensor to Eigen convesion #2953

Refactorize Tensor to Eigen convesion #2953

wangkuiyi commented Jul 18, 2017 •

edited

Loading

QiJune Jul 19, 2017 •

edited

Loading

wangkuiyi Jul 19, 2017

QiJune Jul 19, 2017

reyoung Jul 19, 2017

QiJune Jul 19, 2017

reyoung Jul 19, 2017

QiJune Jul 19, 2017

reyoung Jul 19, 2017

QiJune Jul 19, 2017

reyoung Jul 19, 2017

QiJune Jul 19, 2017

reyoung Jul 19, 2017

reyoung left a comment

reyoung Jul 19, 2017

reyoung Jul 19, 2017

reyoung Jul 19, 2017

reyoung Jul 19, 2017

QiJune Jul 19, 2017

reyoung Jul 19, 2017

QiJune Jul 19, 2017

reyoung Jul 19, 2017 •

edited

Loading

wangkuiyi Jul 20, 2017

reyoung Jul 20, 2017

QiJune Jul 20, 2017

Refactorize Tensor to Eigen convesion #2953

Refactorize Tensor to Eigen convesion #2953

Conversation

wangkuiyi commented Jul 18, 2017 • edited Loading

QiJune Jul 19, 2017 • 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

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

reyoung 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

reyoung Jul 19, 2017 • 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

wangkuiyi commented Jul 18, 2017 •

edited

Loading

QiJune Jul 19, 2017 •

edited

Loading

reyoung Jul 19, 2017 •

edited

Loading