Matrix::MUL operators using and test Daoyuan's Paddle Function, SparseMatrixArg and Function Test #1147

tianbingsz · 2017-01-13T22:59:54Z

Add the following Matrix::Mul operators: (#977)
Use and test Daoyuan's SparseMatrixArg, Function Test.

CpuMatrix::mul(CpuMatrix, CpuMatrix)
GpuMatrix::mul(GpuMatrix, GpuMatrix)
CpuSparseMatrix::mul(CpuMatrix, CpuMatrix)
GpuSparseMatrix::mul(GpuMatrix, GpuMatrix)
CpuMatrix::mul(CpuSparseMatrix, CpuMatrix)
GpuMatrix::mul(GpuSparseMatrix, GpuMatrix)
CpuMatrix::mul(CpuMatrix, CpuSparseMatrix)
GpuMatrix::mul(GpuMatrix, GpuSparseMatrix)

Write new sparse/dense matrix unit test using Function test.

hedaoyuan

主要先看了一下MulFunc的实现。

hedaoyuan · 2017-01-22T03:33:51Z

paddle/function/MulOp.cpp

+
+/**
+ * mul operator
+ * out = scaleT * out + scaleAB*(in1 * in2)


注释还得在加强，要写明白MulFunc支持哪些参数类型，比如Dense，CSR/CSC Sparse等。
另外，calc实现中的几个if逻辑也需要在这里写清楚。

hedaoyuan · 2017-01-22T03:41:51Z

paddle/function/MulOp.cpp

+public:
+  void init(const FuncConfig& config) override {
+    alpha_ = config.get<real>("scaleAB");
+    beta_ = config.get<real>("scaleT");


alpha_, beta_这两个不一定需要。目前Paddle中对于Matrix::mul()的调用只有C = A * B和C = A * B + C两种，分别可以通过设置C参数的ASSIGN_TO/ADD_TO属性来实现。

hedaoyuan · 2017-01-22T04:42:49Z

paddle/function/MulOp.cpp

+ * mul operator
+ * out = scaleT * out + scaleAB*(in1 * in2)
+ *
+ * \param outputs[0]      output matrix, M * N


output matrix, M * N这样的注释也并不好，并没有哪个地方解释M和N分别是什么。

hedaoyuan · 2017-01-22T04:58:14Z

paddle/function/MulOp.cpp

+                    alpha_,
+                    beta_);
+      return;
+    }


还需要增加一个CHECK检查上面几个if都没有走到的逻辑。

hedaoyuan · 2017-01-22T05:04:54Z

paddle/function/BufferArg.h

@@ -269,8 +265,8 @@ class SparseMatrixArg : public BufferArg {
                  const BufferArg& row,
                  const BufferArg& col,
                  size_t nnz,
-                  SparseDataFormat format,
-                  SparseDataType type,
+                  SparseFormat format,


SparseFormat和SparseValueType放在Matrix.h里面并不合适；Matrix.h和SparseMatrix.h里面的数据结构最终是不准备用的，所以可以考虑把这两个TensorType.h里面去。

hedaoyuan · 2017-01-22T05:19:24Z

paddle/function/BufferArg.h

@@ -179,6 +174,7 @@ class BufferArg {
  TensorShape shape_;
  BufferType bufferType_{TENSOR_UNKNOWN};
  ArgType argType_{UNSPECIFIED};
+  bool trans_{false};


BufferArg可以表示一个高维的data，trans_属性对3维或者4维的BufferArg无法很好的解释。
Mul Function里面的transa/transb实际是Function的属性，而不是BufferArg的属性（cblas_sgemm/cublasSgemm也一样）。

trans_ 是数据的属性，matrix, sparse matrix里面是转置矩阵,对于更高order的Tensor，其实是有对应于矩阵转置的generalization的。https://arxiv.org/pdf/1411.1503v1.pdf

hedaoyuan · 2017-01-22T05:23:55Z

paddle/function/MulOp.cpp

+                            const CpuMatrix& b,
+                            real scaleAB,
+                            real scaleT) {
+  CHECK(!out.isTransposed()) << "Not supported";


这里，包括后面几个API中的一些参数CHECK，实际应该是在Function里面检查。如果运行时参数错误，也需要在Function里面就能报出。

hedaoyuan

Approve FunctionTest

hedaoyuan · 2017-01-23T07:05:32Z

paddle/function/FunctionTest.h

  void run() {
    // prepare cpu/gpu arguments
    initInputs();

+    initOutputs();


FunctionCompare可能得加个钩子，uniform(0.001, 1)可能支持不了所有Function的参数初始化。

Good idea. Will do it later for the future unit test cases.

hedaoyuan

另外，我在#1216 (comment)
里面FunctionBase增加ops和check两个接口，在乘法接口里面也可以实现一下。

@hedaoyuan , 好的，我看一下你的PR，然后再开个PR，在MUL里面添加一下这两个接口。
我这个PR比较大一点，已经address了一下你的comments。如果你觉得没什么问题的话，要不先Approve一下？

hedaoyuan · 2017-01-25T13:39:19Z

paddle/function/MulOp.cpp

+  real beta_;
+  bool aTrans_;
+  bool bTrans_;
+  bool cTrans_;


cTrans_不需要吧。cblas_sgemm/cublasSgemm里面也有没有cTrans这个参数的。
另外，alpha_/beta_我觉得还是去掉吧，从目前Paddle的实现来看NeuralNetwork Computation里面并不会需要这样两个参数，去掉也可以稍微简化一点接口的用法。

hedaoyuan · 2017-01-25T13:40:40Z

paddle/function/MulOp.cpp

+  }
+
+  void calc(const BufferArgs& inputs, const BufferArgs& outputs) override {
+    CHECK(!cTrans_) << "output matrix should not be transposed";


以后估计也不会支持cTrans_ == ture，所以直接去掉cTrans_更好。

…::mul(CpuMatrix, CpuMatrix)

and GpuMatrix::mul(CpuMatrix, GpuSparseMatrix)

SparseGpuMatrix::mul(GpuMatrix, GpuMatrix), CpuMatrix::mul(CpuSparseMatrix, CpuMatrix), and GpuMatrix::mul(GpuSparseMatrix, GpuMatrix)

tianbingsz self-assigned this Jan 15, 2017

tianbingsz force-pushed the paddle_func_sparse branch 4 times, most recently from 90a0f3f to 3f09139 Compare January 18, 2017 23:54

tianbingsz changed the title ~~Use and test Daoyuan's SparseMatrixArg.~~ Use and test Daoyuan's SparseMatrixArg for Matrix::MUL operators. Jan 19, 2017

tianbingsz requested a review from hedaoyuan January 19, 2017 06:23

tianbingsz force-pushed the paddle_func_sparse branch from 5020971 to 2074603 Compare January 19, 2017 21:23

tianbingsz changed the title ~~Use and test Daoyuan's SparseMatrixArg for Matrix::MUL operators.~~ Matrix::MUL operators using and test Daoyuan's Paddle Function, SparseMatrixArg and Function Test Jan 19, 2017

tianbingsz force-pushed the paddle_func_sparse branch from 2074603 to 02f5647 Compare January 22, 2017 05:28

hedaoyuan requested changes Jan 22, 2017

View reviewed changes

hedaoyuan reviewed Jan 23, 2017

View reviewed changes

tianbingsz force-pushed the paddle_func_sparse branch from bd54d72 to 6dd017f Compare January 23, 2017 23:03

tianbingsz requested a review from emailweixu January 23, 2017 23:05

hedaoyuan reviewed Jan 25, 2017

View reviewed changes

xutianbing added 13 commits January 25, 2017 11:29

Use and test Daoyuan's SparseMatrixArg.

936301f

add GpuMatrix::mul, CpuMatrix::mul operators

1f0cbcf

Pass Unit test for GpuMatrix::mul(GpuMatrix, GpuMatrix) and CpuMatrix…

2df8eec

…::mul(CpuMatrix, CpuMatrix)

Pass unit test for CpuMatrix::mul(CpuMatrix, CpuSparseMatrix)

1ca2846

and GpuMatrix::mul(CpuMatrix, GpuSparseMatrix)

Pass unit test for SparseCpuMatrix::mul(CpuMatrix, CpuMatrix),

4751cc8

SparseGpuMatrix::mul(GpuMatrix, GpuMatrix), CpuMatrix::mul(CpuSparseMatrix, CpuMatrix), and GpuMatrix::mul(GpuSparseMatrix, GpuMatrix)

clean the code a little bit.

171eaff

clean code a little bit.

9ade63e

clean code in function/MulOp.cpp

316bf75

Support SparseMatrixArg unit test using Daoyuan's new Function Test.

077f936

Add Sparse = dense * dense unit test with Daoyuan's Function test

bc5d7bb

Daoyuan's comments.

b3be735

Further address Daoyuan's comments, clean the code.

999cd14

Daoyuan's comments.

5b1a5c1

tianbingsz force-pushed the paddle_func_sparse branch from e6f0840 to 5b1a5c1 Compare January 25, 2017 19:31

hedaoyuan approved these changes Jan 29, 2017

View reviewed changes

tianbingsz merged commit ecbff68 into PaddlePaddle:develop Jan 30, 2017

wangxicoding pushed a commit to wangxicoding/Paddle that referenced this pull request Dec 9, 2021

Fix readme error of information extraction (PaddlePaddle#1147)

7c460f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matrix::MUL operators using and test Daoyuan's Paddle Function, SparseMatrixArg and Function Test #1147

Matrix::MUL operators using and test Daoyuan's Paddle Function, SparseMatrixArg and Function Test #1147

tianbingsz commented Jan 13, 2017 •

edited

Loading

hedaoyuan left a comment

hedaoyuan Jan 22, 2017

tianbingsz Jan 23, 2017

hedaoyuan Jan 22, 2017

tianbingsz Jan 23, 2017

hedaoyuan Jan 22, 2017

tianbingsz Jan 24, 2017

hedaoyuan Jan 22, 2017

tianbingsz Jan 23, 2017

hedaoyuan Jan 22, 2017

tianbingsz Jan 24, 2017

hedaoyuan Jan 22, 2017

tianbingsz Jan 23, 2017 •

edited

Loading

tianbingsz Jan 23, 2017

hedaoyuan Jan 22, 2017

tianbingsz Jan 23, 2017

hedaoyuan left a comment

hedaoyuan Jan 23, 2017

tianbingsz Jan 24, 2017 •

edited

Loading

hedaoyuan left a comment •

edited by tianbingsz

Loading

hedaoyuan Jan 25, 2017

tianbingsz Jan 25, 2017

hedaoyuan Jan 25, 2017

tianbingsz Jan 25, 2017

Matrix::MUL operators using and test Daoyuan's Paddle Function, SparseMatrixArg and Function Test #1147

Matrix::MUL operators using and test Daoyuan's Paddle Function, SparseMatrixArg and Function Test #1147

Conversation

tianbingsz commented Jan 13, 2017 • edited Loading

hedaoyuan 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

tianbingsz Jan 23, 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

hedaoyuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianbingsz Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

hedaoyuan left a comment • edited by tianbingsz 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

tianbingsz commented Jan 13, 2017 •

edited

Loading

tianbingsz Jan 23, 2017 •

edited

Loading

tianbingsz Jan 24, 2017 •

edited

Loading

hedaoyuan left a comment •

edited by tianbingsz

Loading