add new api fill_diagonal_tensor_ #34515

zhiboniu · 2021-07-30T09:22:04Z

PR types

New features

PR changes

APIs

Describe

add new op/api fill_diagonal_tensor_

Examples:
    .. code-block:: python
        import paddle
        x = paddle.ones((4, 3)) * 2
        y = paddle.ones((3,))
        x.fill_diagonal_tensor_(y)
        print(x.tolist())   #[[1.0, 2.0, 2.0], [2.0, 1.0, 2.0], [2.0, 2.0, 1.0], [2.0, 2.0, 2.0]]

paddle-bot-old · 2021-07-30T09:22:33Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

python/paddle/tensor/manipulation.py

jeff41404 · 2021-08-02T13:07:33Z

python/paddle/tensor/manipulation.py

+    assert len(inshape) >= 2, ('Tensor dims should >= 2 in fill_diagmat_')
+    dim1 %= len(inshape)
+    dim2 %= len(inshape)
+    assert dim1 < dim2, ("dim1 should > dim2 in fill_diagmat_")


看这里的逻辑，报错中的 > 应该为 <

done，笔误，已修改

jeff41404 · 2021-08-06T04:00:42Z

paddle/fluid/operators/fill_diagmat_op.cc

+    framework::TensorCopy(*xin, ctx.GetPlace(), out);
+
+    T *out_data = out->mutable_data<T>(ctx.GetPlace());


常规做法是先 out->mutable_data 分配内存后再使用 TensorCopy，这里反过来是否有特殊考虑

done, TensorCopy中也有out->mutable_data，所以其实都可以。不过也已经修改。

jeff41404 · 2021-08-06T04:01:03Z

paddle/fluid/operators/fill_diagmat_op.cu

+    framework::TensorCopy(*xin, ctx.GetPlace(), out);
+
+    T *out_data = out->mutable_data<T>(ctx.GetPlace());


常规做法是先 out->mutable_data 分配内存后再使用 TensorCopy，这里反过来是否有特殊考虑

done，同上

sneaxiy

Suggestion: you can use platform::ForRange to unify the CPU and GPU codes.

sneaxiy · 2021-08-06T13:53:23Z

paddle/fluid/operators/fill_diagonal_tensor_op.cc

+              "Tensor, the output tensor, with the same shape and data type "
+              "as input(x)");
+    AddAttr<int>("dim1", "the first dim to figure out the diagonal")
+        .SetDefault(-2);


sneaxiy · 2021-08-06T13:53:37Z

paddle/fluid/operators/fill_diagonal_tensor_op.cc

+    AddAttr<int>("dim1", "the first dim to figure out the diagonal")
+        .SetDefault(-2);
+    AddAttr<int>("dim2", "the second dim to figure out the diagonal")
+        .SetDefault(-1);


sneaxiy · 2021-08-06T13:54:39Z

paddle/fluid/operators/fill_diagonal_tensor_op.cc

+                 "offset to up-right corner; negtive means offset to "
+                 "bottom-left corner")
+        .SetDefault(0);
+    AddAttr<int>("matrows",


It seems that this attribute is not necessary.

used for backward memory allocate

I know the purpose that you add this attribute. I mean, this attribute is not necessary, because it can be calculated by other informations (i.e., the dimensions of the input tensors and the values of other attributes). It is not orthogonal with other informations.

sneaxiy · 2021-08-06T13:56:15Z

paddle/fluid/operators/fill_diagonal_tensor_op.cc

+  }
+};
+
+class FillDiagonalTensorOpVarTypeInference


It seems that this class is not necessary because X and Out must be both Tensor. Please remove it.

算子开发注意事项中要求的注册这个函数：
框架没有提供默认的op_infer_var_type方法，用户需要根据实际情况添加op_infer_var_type。严格来说每个Op都应该注册一个InferVarType，op_infer_var_type根据输入的Var的type和dtype推断输出Var的type和dtype。

sneaxiy · 2021-08-06T13:57:03Z

paddle/fluid/operators/fill_diagonal_tensor_op.cc

+    auto *xin = ctx.Input<framework::Tensor>("X");
+
+    T *out_data = out->mutable_data<T>(ctx.GetPlace());
+    T *fill_data = const_cast<T *>(srctensor->data<T>());


Why const_cast? I do not think that you should change fill_data.

sneaxiy · 2021-08-06T14:51:12Z

paddle/fluid/operators/fill_diagonal_tensor_op.cu

+#ifdef __HIPCC__
+    hipMalloc(reinterpret_cast<void **>(&memory_block_cu),
+              sizeof(int) * (2 + fill_dims[0]));
+    hipMemcpy(memory_block_cu, memory_block, sizeof(int) * (2 + fill_dims[0]),


Use memory::Copy instead of hipMemcpy/cudaMemcpy.

sneaxiy · 2021-08-06T15:03:17Z

paddle/fluid/operators/fill_diagonal_tensor_op.cu

+
+    dim3 kBlockDim(std::min(int64_t(new_dims[0]), kMaxBlockDim),
+                   std::min(int64_t(new_dims[1]), kMaxBlockDim), 1);
+    fill_diagonal_tensor_kernel<T><<<1, kBlockDim, 0>>>(


I do not know why you use this dimension setting. I prefer to use both gridDim and blockDim. The gridDim can be up to 2^27 usually.

Also, add testings of large dimensions.

sneaxiy · 2021-08-06T15:05:05Z

python/paddle/fluid/tests/unittests/test_tensor_fill_diagmat_.py

+
+class TensorFillDiagTensor_Test(unittest.TestCase):
+    def test_dim2(self):
+        expected_np = np.array(


Add testing with larger dimensions.

sneaxiy · 2021-08-06T15:05:47Z

python/paddle/tensor/manipulation.py

@@ -37,6 +37,43 @@
 __all__ = []


+@dygraph_only
+def fill_diagonal_tensor_(x, src_tensor, dim1=0, dim2=1, offset=0):


It seems that we can have another API named fill_diagonal_tensor (non-inplace version). This API can be used in both static and dygraph mode.

sneaxiy · 2021-08-06T15:06:09Z

python/paddle/tensor/manipulation.py

+        src_tensor = src_tensor.reshape([1, -1])
+
+    matrows = 1
+    for dim in src_tensor.shape[:-1]:


Not need this attribute.

sneaxiy · 2021-08-16T14:17:44Z

paddle/fluid/operators/fill_diagonal_tensor_op.cc

+ public:
+  void Make() override {
+    AddComment(R"DOC(Fill replace operator
+                Fill the diagonal matrix of an tensor with `y` Tensor.


As discussed offline, not matrix.

sneaxiy · 2021-08-16T14:18:02Z

paddle/fluid/operators/fill_diagonal_tensor_op.cc

+                Fill the diagonal matrix of an tensor with `y` Tensor.
+                )DOC");
+    AddInput("X", "(Tensor) The input tensor.");
+    AddInput("y", "(Tensor) The input tensor to filled in.");


to filled -> to fill.

y -> SrcTensor.

done, As discussed, "y" is accepted

sneaxiy · 2021-08-16T14:27:55Z

python/paddle/tensor/manipulation.py

@@ -37,6 +37,128 @@
 __all__ = []


+@dygraph_only
+def fill_diagonal_tensor_(x, y, dim1=0, dim2=1, offset=0, name=None):
+    """


Why not unify most of the codes of fill_diagonal_tensor_ and fill_diagonal_tensor. Too many duplicate codes....

sneaxiy · 2021-08-16T14:28:36Z

python/paddle/fluid/tests/unittests/test_tensor_fill_diagonal_tensor_.py

+import paddle
+
+
+class TensorFillDiagTensor_Test(unittest.TestCase):


No tests with the case dim1 > dim2.

Why not use OpTest?

done, add both

sneaxiy

LGTM.

XiaoguangHu01 · 2021-08-30T06:37:13Z

python/paddle/tensor/manipulation.py

@@ -86,6 +86,112 @@ def fill_diagonal_(x, value, offset=0, wrap=False, name=None):
 setattr(core.VarBase, 'fill_diagonal_', fill_diagonal_)


+def fill_diagonal_tensor_kernel(x, y, offset=0, dim1=0, dim2=1, inplace=False):


kernel是c++底层的概念，python层的API需要避免用这个名字，避免概念混淆。
建议用其他名字，比如_fill_diagonal_tensor_impl之类，表示具体的实现implement

XiaoguangHu01

LG API

jzhang533 · 2021-09-06T08:53:56Z

python/paddle/tensor/manipulation.py

+    Returns:
+        Tensor: Tensor with diagonal filled with y.
+
+    Returns type:


returns type这个在文档说明中不需要吧？

嗯，下个pr里改了

jzhang533 · 2021-09-06T08:54:00Z

python/paddle/tensor/manipulation.py

+    Returns:
+        Tensor: Tensor with diagonal filled with y.
+
+    Returns type:


returns type这个在文档说明中不需要吧？

嗯，下个pr里改了

jzhang533

LGTM

jeff41404

lgtm

jeff41404 reviewed Aug 2, 2021

View reviewed changes

python/paddle/tensor/manipulation.py Show resolved Hide resolved

jeff41404 reviewed Aug 2, 2021

View reviewed changes

jeff41404 reviewed Aug 6, 2021

View reviewed changes

sneaxiy reviewed Aug 6, 2021

View reviewed changes

sneaxiy reviewed Aug 16, 2021

View reviewed changes

zhiboniu changed the title ~~add api fill_diagmat_inplace~~ add new api fill_diagonal_tensor_ Aug 17, 2021

sneaxiy previously approved these changes Aug 25, 2021

View reviewed changes

XiaoguangHu01 reviewed Aug 30, 2021

View reviewed changes

add api_op fill_diagonal_tensor

398f047

zhiboniu dismissed sneaxiy’s stale review via 398f047 August 30, 2021 06:41

XiaoguangHu01 approved these changes Sep 6, 2021

View reviewed changes

jzhang533 reviewed Sep 6, 2021

View reviewed changes

jzhang533 approved these changes Sep 6, 2021

View reviewed changes

raindrops2sea approved these changes Sep 10, 2021

View reviewed changes

jeff41404 approved these changes Sep 10, 2021

View reviewed changes

jeff41404 merged commit 98d047d into PaddlePaddle:develop Sep 10, 2021

AnnaTrainingG pushed a commit to AnnaTrainingG/Paddle that referenced this pull request Sep 29, 2021

add api_op fill_diagonal_tensor (PaddlePaddle#34515)

888b98d

		framework::TensorCopy(*xin, ctx.GetPlace(), out);

		T *out_data = out->mutable_data<T>(ctx.GetPlace());

		import paddle


		class TensorFillDiagTensor_Test(unittest.TestCase):

		@@ -86,6 +86,112 @@ def fill_diagonal_(x, value, offset=0, wrap=False, name=None):
		setattr(core.VarBase, 'fill_diagonal_', fill_diagonal_)


		def fill_diagonal_tensor_kernel(x, y, offset=0, dim1=0, dim2=1, inplace=False):

add new api fill_diagonal_tensor_ #34515

add new api fill_diagonal_tensor_ #34515

Conversation

zhiboniu commented Jul 30, 2021 • edited Loading

PR types

PR changes

Describe

paddle-bot-old bot commented Jul 30, 2021

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

sneaxiy 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

sneaxiy Aug 11, 2021 • 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

sneaxiy Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sneaxiy Aug 6, 2021 • 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

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

sneaxiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiaoguangHu01 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

jzhang533 left a comment

Choose a reason for hiding this comment

jeff41404 left a comment

Choose a reason for hiding this comment

zhiboniu commented Jul 30, 2021 •

edited

Loading

sneaxiy Aug 11, 2021 •

edited

Loading

sneaxiy Aug 6, 2021 •

edited

Loading

sneaxiy Aug 6, 2021 •

edited

Loading