[triu_indices] add triu_indices_op #45168

Rayman96 · 2022-08-16T01:26:38Z

PR types

New features

PR changes

APIs

Describe

为 Paddle 新增 triu_indices API, 包括 CPU&GPU kernel，python API以及单元测试

RFC的PR链接: PaddlePaddle/community#207
DOC的PR链接: PaddlePaddle/docs#5161

paddle-bot · 2022-08-16T01:26:42Z

你的PR提交成功，感谢你对开源项目的贡献!
请关注后续CI自动化测试结果，详情请参考Paddle-CI手册。
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

Rayman96 · 2022-08-17T13:38:34Z

可否请教下目前CI的三个失败。
CI-APPROVAL和CI-Static-Check好像是非代码层面的问题；
CI-Kunlun的错误没有理解日志显示的问题

betterpig · 2022-08-18T02:26:17Z

可否请教下目前CI的三个失败。
CI-APPROVAL和CI-Static-Check好像是非代码层面的问题；
CI-Kunlun的错误没有理解日志显示的问题

CI-APPROVAL和CI-Static-Check等其他流水线通过后，需要找人review并approve。
CI-Kunlun是网络问题。已经重跑了

Rayman96 · 2022-08-18T03:55:18Z

可否请教下目前CI的三个失败。
CI-APPROVAL和CI-Static-Check好像是非代码层面的问题；
CI-Kunlun的错误没有理解日志显示的问题

CI-APPROVAL和CI-Static-Check等其他流水线通过后，需要找人review并approve。 CI-Kunlun是网络问题。已经重跑了

多谢回复，目前除CI-APPROVAL和CI-Static-Check外都已通过，辛苦审核review

betterpig · 2022-08-18T07:34:27Z

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

+    def test_static(self):
+        places = [
+            paddle.CPUPlace(), paddle.fluid.CUDAPlace(0)
+        ] if fluid.core.is_compiled_with_cuda() else [paddle.CPUPlace()]


现在有专门的CPU流水线和GPU流水线，为了减少单测时间，当为GPU版本时只跑GPUplace即可。所以后面的循环也是不必要的。下同。

betterpig · 2022-08-18T07:48:21Z

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

+                                             paddle.static.Program()):
+                data1 = paddle.triu_indices(4, 4, -1)
+                exe1 = paddle.static.Executor(place)
+                result1 = exe1.run(feed={}, fetch_list=[data1])


这里变量加上后缀1的用意是什么

这两行应放到guard外面。下同。

exe1 = paddle.static.Executor(place) result1 = exe1.run(feed={}, fetch_list=[data1])

第一条已修改命名；第二条放到guard外后会报错，就保持缩进没有动了

这里变量加上后缀1的用意是什么

这两行应放到guard外面。下同。

exe1 = paddle.static.Executor(place) result1 = exe1.run(feed={}, fetch_list=[data1])

将后两行移出后会报错显示, 本地和流水线都可复现：

Traceback (most recent call last):
File "test_triu_indices_op.py", line 73, in test_static
result = exe.run(feed={}, fetch_list=[data])
File "/usr/local/lib/python3.7/dist-packages/paddle/fluid/executor.py", line 1307, in run
six.reraise(*sys.exc_info())
File "/usr/local/lib/python3.7/dist-packages/six.py", line 703, in reraise
raise value
File "/usr/local/lib/python3.7/dist-packages/paddle/fluid/executor.py", line 1303, in run
return_merged=return_merged)
File "/usr/local/lib/python3.7/dist-packages/paddle/fluid/executor.py", line 1582, in _run_impl
return_numpy)
File "/usr/local/lib/python3.7/dist-packages/paddle/fluid/executor.py", line 546, in run
fetch_list)._move_to_list()
RuntimeError: (NotFound) triu_indices_1.tmp_0 not in VariableScope.
[Hint: Expected HasVar(name) == true, but received HasVar(name):0 != true:1.] (at /paddle/paddle/fluid/framework/new_executor/new_executor_defs.cc:677)

betterpig · 2022-08-18T07:49:41Z

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

+                exe1 = paddle.static.Executor(place)
+                result1 = exe1.run(feed={}, fetch_list=[data1])
+            expected_result1 = np.triu_indices(4, -1, 4)
+            self.assertTrue(np.allclose(result1, expected_result1))


使用assertTrue在结果出现diff时报错不直观，无法看出diff在哪里，差了多少。改为np.testing.assert_array_equal。下同

@Rayman96 ，这里可参考 https://github.com/PaddlePaddle/community/blob/master/rfcs/CodeStyle/20220805_code_style_improvement_for_unittest.md#1%E7%9B%B8%E5%85%B3%E8%83%8C%E6%99%AF ，是近期一个正在进行的一项修改

另外，目测这里的静态图返回值是没有 unpack 的，替换成 np.testing 下的函数可能会报 shape 不匹配的问题，静态图返回值是一个 list，因此如果想要取得其结果应该 unpack 下，修改方式可参考正在进行中的 #45213（只是目测，也可能我看错了 😂）

修改后 PR-CI-APPROVAL 应可通过

好的，我根据参考链接修改此处判断

已修改，可通过PR-CI-APPROVAL

betterpig · 2022-08-18T07:53:54Z

python/paddle/tensor/creation.py

+            data1 = paddle.triu_indices(4,4,0)
+            print(data1)
+            # [[0, 0, 0, 0, 1, 1, 1, 2, 2, 3],
+            # [0, 1, 2, 3, 1, 2, 3, 2, 3, 3]]


内层中括号对齐-->

# [[0, 0, 0, 0, 1, 1, 1, 2, 2, 3], # [0, 1, 2, 3, 1, 2, 3, 2, 3, 3]]

几条example都已修改

betterpig · 2022-08-18T07:55:02Z

python/paddle/tensor/creation.py

+        col = row
+
+    if not isinstance(offset, int):
+        raise TypeError("offset should be a  int")


be a int. 空一格

betterpig · 2022-08-18T08:39:20Z

paddle/phi/kernels/gpu/triu_indices_kernel.cu

+  auto b = -1 - f;
+  auto cX4 = x << 3;  // 4 * c = 4 * (2x) = 8x;
+  *row = resolve_root_int<T>(b, cX4, x, -1);
+  *col = x - ((f - *row + 1) * *row >> 1) + *row;


这里最好加上（）突出一下运算优先级。

…into for_api_triu

Rayman96 · 2022-08-23T02:29:22Z

麻烦 @betterpig 大佬看下，这里的col允许输入为None以获得默认正方形矩阵是合理的吗

合理，这里是对齐了numpy的接口，简化方阵的输入。就按支持col为None来写接口和文档。

好的

Rayman96 · 2022-08-23T06:53:09Z

@betterpig RFC已修改，麻烦审核。PaddlePaddle/community#219

Rayman96 · 2022-08-23T10:29:26Z

@betterpig creation.py 接口已修改麻烦审核
@SigureMo 中英文的文档已修改麻烦审核 https://github.com/PaddlePaddle/docs/pull/5161。

SigureMo

LGTM for docs

SigureMo · 2022-08-23T13:17:26Z

python/paddle/tensor/creation.py

+            - If offset = 0, all elements on and below the main diagonal are retained.
+            - If offset > 0, include just as few diagonals above the main diagonal.
+            - If offset < 0, excludes just as few diagonals below the main diagonal.


这里也上下各加一个空行吧，刚才在 review 其他文档发现英文这个上下都有空行应该也是必需的

另外 offset 现在也有默认值了，这里需要加上默认值的描述

这里也上下各加一个空行吧，刚才在 review 其他文档发现英文这个上下都有空行应该也是必需的

另外 offset 现在也有默认值了，这里需要加上默认值的描述

空行已添加
offset的默认值之前在1938行已有描述

😂 报歉是我眼瞎了

Rayman96 · 2022-08-24T00:51:00Z

@betterpig @SigureMo
修改后CI已通过，文档看起来也ok的，辛苦大佬们再审核看是否可以通过

SigureMo

LGTM for docs，麻烦 @Ligoml 最终确认下

Ligoml · 2022-08-24T07:08:21Z

python/paddle/tensor/creation.py

+    Return the indices of the upper triangular part of the 2-D matrix
+    whose row and col is known. Indices are ordered based on row and then columns.
+    The upper triangular part of the matrix is defined as the elements on
+    and above the diagonal.


这里有一些报错，应该需要在 API 介绍和 Args: 之间空一行

我修改下

Ligoml

LGTM for docs

jeff41404 · 2022-08-24T09:55:46Z

python/paddle/tensor/creation.py

@@ -1923,3 +1923,88 @@ def tril_indices(row, col, offset=0, dtype='int64'):
                             'dtype': dtype
                         })
    return out
+
+
+def triu_indices(row, col=None, offset=0, dtype='int64'):


the title of rfc should be paddle.triu_indices instead of paddle.tril_indices?

the title of rfc should be paddle.triu_indices instead of paddle.tril_indices?

不好意思我写错了。已重新提交rfc的PR PaddlePaddle/community#224

jeff41404 · 2022-08-24T10:00:46Z

python/paddle/tensor/creation.py

+        return out
+
+    if _in_legacy_dygraph():
+        out = _C_ops.triu_indices('rows', row, 'cols', col, 'offset', offset,


In order to reuse documents, the name of python API parameters should be consistent with C++ API's, so should be row and col, and paddle.tril_indices have same problem which shoud be fix.

In order to reuse documents, the name of python API parameters should be consistent with C++ API's, so should be row and col, and paddle.tril_indices have same problem which shoud be fix.

triu_indices的代码中已统一将 rows cols 替换成 row col。 tril_indices应该另提一个PR修改？就不在这个PR里做调整了

jeff41404 · 2022-08-24T10:01:51Z

python/paddle/tensor/creation.py

+                             'rows': row,
+                             'cols': col,


same with L1992

jeff41404 · 2022-08-24T10:24:28Z

paddle/phi/kernels/gpu/triu_indices_kernel.cu

+
+template <typename T>
+__device__ inline int resolve_root_int(int b, int cX4, int x, int32_t sign) {
+  int bXb_cX4 = b * b - cX4;


to multiply two int32 numbers, it's better to use int64_t or long long int to avoid overflow?

Rayman96 · 2022-08-25T01:01:23Z

@jeff41404 提到的问题已修改并通过CI，劳烦审核

jeff41404

LGTM

Ligoml

LGTM for docs

[triu_indices] add triu_indices_op

41125cd

paddle-bot bot added contributor External developers status: proposed labels Aug 16, 2022

Rayman96 mentioned this pull request Aug 16, 2022

【PaddlePaddle Hackathon 第三期】任务总览 #43938

Closed

[triu_indices] bug fix

657353f

luotao1 added the PaddlePaddle Hackathon label Aug 16, 2022

paddle-bot bot removed the status: proposed label Aug 16, 2022

luotao1 assigned luotao1, Ligoml and betterpig Aug 16, 2022

luotao1 requested a review from betterpig August 16, 2022 02:38

Rayman96 added 3 commits August 16, 2022 12:30

[triu_indices] code style fix

481fb0c

rename file

d487c3d

modify code style

b25240f

betterpig requested changes Aug 18, 2022

View reviewed changes

Rayman96 added 11 commits August 18, 2022 21:25

modify code

665326b

update yaml

43608d5

Merge branch 'develop' into for_api_triu

1b7ad9d

modify code style

7c75fd7

Merge branch 'for_api_triu' of https://github.com/Rayman96/Paddle_ray …

cf17595

…into for_api_triu

modify code style

a38d03f

modify code style

8185cf3

modify code

4467566

modify code

5109e24

modify example

101920e

modify example

11d8d7f

change default value for col

63dd431

modify doc

d3c8854

SigureMo previously approved these changes Aug 23, 2022

View reviewed changes

add blank lines

b194791

Rayman96 dismissed SigureMo’s stale review via b194791 August 23, 2022 13:23

modify doc

1c1220a

betterpig previously approved these changes Aug 24, 2022

View reviewed changes

SigureMo previously approved these changes Aug 24, 2022

View reviewed changes

Ligoml reviewed Aug 24, 2022

View reviewed changes

modify doc

5b4af77

Rayman96 dismissed stale reviews from SigureMo and betterpig via 5b4af77 August 24, 2022 07:14

Ligoml previously approved these changes Aug 24, 2022

View reviewed changes

jeff41404 reviewed Aug 24, 2022

View reviewed changes

change var name

d1d8454

Rayman96 dismissed Ligoml’s stale review via d1d8454 August 24, 2022 12:45

format code

d7b2414

jeff41404 approved these changes Aug 25, 2022

View reviewed changes

Ligoml approved these changes Aug 25, 2022

View reviewed changes

luotao1 mentioned this pull request Aug 25, 2022

[Eager] delete final state pre-name #45306

Merged

luotao1 merged commit a410c39 into PaddlePaddle:develop Aug 25, 2022

SigureMo mentioned this pull request Sep 7, 2022

【社区治理】SigureMo 发起 Committer 身份申请 #45841

Closed

[triu_indices] add triu_indices_op #45168

[triu_indices] add triu_indices_op #45168

Conversation

Rayman96 commented Aug 16, 2022 • edited Loading

PR types

PR changes

Describe

paddle-bot bot commented Aug 16, 2022

Rayman96 commented Aug 17, 2022

betterpig commented Aug 18, 2022 • edited Loading

Rayman96 commented Aug 18, 2022 • 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

Rayman96 Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

将后两行移出后会报错显示, 本地和流水线都可复现：

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SigureMo Aug 18, 2022 • 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

Rayman96 commented Aug 23, 2022

Rayman96 commented Aug 23, 2022

Rayman96 commented Aug 23, 2022

SigureMo 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

Rayman96 commented Aug 24, 2022

SigureMo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ligoml 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

Rayman96 commented Aug 25, 2022

jeff41404 left a comment

Choose a reason for hiding this comment

Ligoml left a comment

Choose a reason for hiding this comment

Rayman96 commented Aug 16, 2022 •

edited

Loading

betterpig commented Aug 18, 2022 •

edited

Loading

Rayman96 commented Aug 18, 2022 •

edited

Loading

Rayman96 Aug 19, 2022 •

edited

Loading

SigureMo Aug 18, 2022 •

edited

Loading