【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API -part #59973

megemini · 2023-12-13T07:13:15Z

PR types

New features

PR changes

APIs

Description

【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API

RFC：

v1 : PaddlePaddle/community#784
v2 : PaddlePaddle/community#790

~~p.s. 待 RFC 评审之后完善代码与测试～~~

paddle-bot · 2023-12-13T07:13:20Z

你的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.

… hack5_28

megemini · 2023-12-18T09:27:08Z

@zoooo0820 CI 应该没啥问题了请评审～谢谢！：）

zoooo0820 · 2023-12-18T10:29:06Z

test/legacy_test/test_slice_scatter.py

+paddle.enable_static()
+
+RTOL = {'float32': 1e-03, 'float64': 1e-05}
+ATOL = {'float32': 1e-03, 'float64': 1e-05}


这里设置的两个阈值相对数据类型本身是否宽容度高了些，能否用默认参数测试

嗯这里没必要单独设置，已删掉～

zoooo0820 · 2023-12-18T10:30:17Z

python/paddle/tensor/manipulation.py

+    attrs['dtype'] = dtype
+
+    value = value.astype(dtype)
+    inputs["ValueTensor"] = value


仅静态图分支用到的参数，比如inputs / attrs等，挪到静态图分支下吧

zoooo0820 · 2023-12-18T10:33:19Z

test/legacy_test/test_slice_scatter.py

+
+class TestSliceScatterApiFloat32(TestSliceScatterApi):
+    def init_dtype(self):
+        self.dtype = 'float32'


也补充下其他数据类型的测试吧，应该不用特别设置atol /rtol? 可以关注下fp16和bf16的单测通过情况

已增加测试用例～

另外，由于旧 ir 有些数据类型不支持，所以在测试用例里面单列出来了～

zoooo0820

LGTM

sunzhongkai588 · 2023-12-19T06:23:33Z

python/paddle/tensor/manipulation.py

@@ -6749,3 +6749,108 @@ def select_scatter(x, values, axis, index, name=None):
        )

        return output
+
+
+def slice_scatter(x, value, axis=0, start=None, stop=None, step=1, name=None):


这里的默认值好像和下面的参数描述没对上？（start 和 stop）

赞！🤙🤙🤙

确实不一样，当时也很纠结要怎么写～

因为这里如果不写参数的话，如：slice_scatter(x, value) ，那么 start 为 None，程序里面会转换为 0，stop 为 None，会转换为 x.shape[axis] ～

参考 python 中 range 的写法 :

https://docs.python.org/3/library/stdtypes.html#range

If the start argument is omitted, it defaults to 0.

有没有什么建议的写法？谢谢！：）

因为这里如果不写参数的话，如：slice_scatter(x, value) ，那么 start 为 None，程序里面会转换为 0，stop 为 None，会转换为 x.shape[axis] ～

个人建议，可以直接在start 和 stop的参数描述里说清楚，当为 None 时，会出现的情况，且默认值为 None。

def ... 这边还是保留参数。宗旨就是减少读者误解

sunzhongkai588

LGTM

luotao1 · 2023-12-20T06:52:47Z

可以提供中文文档

jeff41404 · 2023-12-20T07:37:21Z

python/paddle/tensor/manipulation.py

+        axis (int) : the dimension to insert the value. Default is 0.
+        start (int, optional) : the start index of where to insert. Default is `None` which will be converted to `0`.
+        stop (int, optional) : the stop index of where to insert. Default is `None` which will be converted to `x.shape[axis]`.
+        step (int, optional) : the step for each insert. Default is 1.


Because operator of set_valuecan support writing data according to multiple axes, starts, ends and steps, it is better that the parameters of axis, start, stop, step here can support int and list of int ？ making this API more user-friendly and powerful

If modified according to the above suggestions, the parameter naming suggestion are axes, starts, ends and strides, which is consistent with API of paddle.slice and paddle.strided_slice. and rfc should also be modified.

No ~~~

list of int of axis, start, stop and step for slice_scatter NOT make sense ~

For instance:

x is a tensor of shape [8, 8], value is [8, 2], then the axis can ONLY be 1. If axis=0 , the values of value in axis 1 of length 2 can not be fitted into x of length 8. In other words, list of int can only be useful of x and value the same shape, which is not that useful ~~~ BTW, torch not support list of int either ~~~

From a functional perspective, the functionality of this API should be dual to paddle.strided_slice. Because the former is for writing data, while the latter is for reading data, so supporting list of int is more natural, and the underlying operator of set_value also supports it. As for the example you gave, if the shape of value cannot be broadcasted to the shape after the slice, an error should be reported (by set_value). Additionally, not all of design of torch are reasonable, but how to make it better for users is what we pursue

From a functional perspective, the functionality of this API should be dual to paddle.strided_slice. Because the former is for writing data, while the latter is for reading data, so supporting list of int is more natural, and the underlying operator of set_value also supports it. As for the example you gave, if the shape of value cannot be broadcasted to the shape after the slice, an error should be reported (by set_value).

Not agree ~

Especially treat slice_scatter as a dual to paddle.strided_slice.

There should be a powerful API set_value or set_value_with_tensor, not slice_scatter ~

We should not bother users to map slice_scatter from torch, mindspore or other framework to ours, and not mess the API up with more options just because we could support it ~

Additionally, not all of design of torch are reasonable, but how to make it better for users is what we pursue

Agree and always !!!

My opinion is that regardless of whether API name is set_value or slice_scatter, our goal is to have an API that is functionally dual to paddle.strided_slice. Here, this PR is just a subset of this feature, and we don't need to create two APIs with the same parameters. The only difference is that one supports int and the other supports list of int

jeff41404 · 2023-12-20T07:48:26Z

python/paddle/tensor/manipulation.py

+    exp_shape = [*x_shape[:axis], len(index), *x_shape[axis + 1 :]]
+    if tuple(exp_shape) != tuple(value_shape):
+        raise ValueError(
+            "The value.shape should be same of [*x_shape[:axis], len(index), *x_shape[axis+1:]],"
+            f"but got value.shape of {value.shape} and slice shape {exp_shape}."
+        )


we also should support the shape of value can broadcast to the shape of exp_shape, not only exactly equal, and this check will be completed in set_value, so there is no need to add this check here, which actually limits the functionality.

OK！ remove the check and add broadcast test cases ~

jeff41404 · 2023-12-20T07:52:13Z

python/paddle/tensor/manipulation.py

+            >>> x = paddle.zeros((8, 8))
+            >>> value = paddle.ones((8, 2))
+            >>> res = paddle.slice_scatter(x, value, axis=1, start=2, stop=6, step=2)
+            >>> print(res)
+            Tensor(shape=[8, 8], dtype=float32, place=Place(cpu), stop_gradient=True,
+            [[0., 0., 1., 0., 1., 0., 0., 0.],
+             [0., 0., 1., 0., 1., 0., 0., 0.],
+             [0., 0., 1., 0., 1., 0., 0., 0.],
+             [0., 0., 1., 0., 1., 0., 0., 0.],
+             [0., 0., 1., 0., 1., 0., 0., 0.],
+             [0., 0., 1., 0., 1., 0., 0., 0.],
+             [0., 0., 1., 0., 1., 0., 0., 0.],
+             [0., 0., 1., 0., 1., 0., 0., 0.]])


the example code needs to add some cases to facilitate user understanding and not be too similar to competitors

megemini · 2023-12-20T09:58:21Z

Update 20231220

删除掉 shape 的 check
增加 broadcast 的单测
增加示例程序

关于 list of int 的 axis 等参数，个人感觉还是没必要支持，原因：

场景应用很少，没必要，而且会对用户的使用产生困惑～
torch 和 mindspore 都不支持，应该也算是行业惯例～

如何？

megemini · 2023-12-21T12:59:03Z

关于这个 API 是否支持 list of int ，最后说一下我的看法～

jeff 大佬认为 slice_scatter 与 strided_slice 是 对偶 关系，因此需要支持～这个理由实在无法说服我～

首先，我从来不觉得 torch 的设计就是对的，很多 PR 都可以表明我的态度，这里不再赘述。
其次，是否 对偶 ，个人觉得应该从用户的角度出发，而不是我们决定。
比如说 hsplit 和 hstack 虽然参数不同，但我觉得可以是 功能对偶 ，因为从名字上就可以很直观的理解。
而 slice_scatter 与 strided_slice 怎么看都看不出哪儿 对偶 了，至少刚接触这两个 API 的我很难想到 对偶 ... ... 🤣🤣🤣
如果一定要给 strided_slice 找个 对偶 的名字，我觉得 strided_scatter 更合适，strided 表示这两个 API 是步进的，slice 表示 read，scatter 表示 write ～
为了 对偶 和极少用到的情况而给用户带来困惑，和抛异常，实在不能赞同～～～
最后，如果内部研发一致支持 list of int 的用法，这里改起来不要太简单 ... ... 😆😆😆

不用我单独翻译成英文再写一遍吧～ 😜😜😜

@zoooo0820 @jeff41404

megemini · 2023-12-24T03:35:07Z

Update 20231224

修改 axis, start, stop, step 为 axes, starts, ends, strides，与 strided_slice 参数保持一致
修改为 list of int 后，去掉默认参数值
修改并增加测试用例

v2.0 RFC PaddlePaddle/community#790

@jeff41404 @zoooo0820 请评审～

jeff41404 · 2023-12-26T09:48:10Z

关于这个 API 是否支持 list of int ，最后说一下我的看法～
jeff 大佬认为 slice_scatter 与 strided_slice 是对偶关系，因此需要支持～这个理由实在无法说服我～

其次，是否 对偶 ，个人觉得应该从用户的角度出发，而不是我们决定。
比如说 hsplit 和 hstack 虽然参数不同，但我觉得可以是 功能对偶 ，因为从名字上就可以很直观的理解。
而 slice_scatter 与 strided_slice 怎么看都看不出哪儿 对偶 了，至少刚接触这两个 API 的我很难想到 对偶 ... ... 🤣🤣🤣
如果一定要给 strided_slice 找个 对偶 的名字，我觉得 strided_scatter 更合适，strided 表示这两个 API 是步进的，slice 表示 read，scatter 表示 write ～
为了 对偶 和极少用到的情况而给用户带来困惑，和抛异常，实在不能赞同～～～

The main issue you mentioned here is a naming issue. This DOES exist, the root cause is not that the name of slice_scatter is not good (of course changed to slice_assign may be better). but paddle.strided_slice should actually be paddle.slice (replacing the existing paddle.slice) which will be solved, so the problem you mentioned no longer exists and will not affect the design here.
As for the "rarely used cases", this judgment cannot be made unless there is sufficient data support for slicing multiple axes will be rarely used. Otherwise, we should try to provide rich functionality as much as possible.

jeff41404

LGTM

* [Add] hack5 28 api * [Change] use set_value op * [Change] values to value * [Fix] resolve conflict * [Update] add test cases * [Fix] code example * [Add] dtype test cases * [Change] start/stop docstring * [Change] fix start/stop docstring * [Change] broadcast value to exp_shape * [Change] axes with list of int * [Add] as tensor test case * [Change] code style

[Add] hack5 28 api

e85068e

paddle-bot bot added the contributor External developers label Dec 13, 2023

luotao1 added the PaddlePaddle Hackathon label Dec 13, 2023

luotao1 assigned luotao1 and zoooo0820 Dec 13, 2023

luotao1 mentioned this pull request Dec 13, 2023

【PaddlePaddle Hackathon 5th】开源贡献个人挑战赛 #57262

Closed

megemini added 6 commits December 14, 2023 13:12

[Change] use set_value op

009fd46

[Change] values to value

9a1ad5e

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

53dc749

… hack5_28

[Fix] resolve conflict

f729a4e

[Update] add test cases

b5371ac

[Fix] code example

5d82cbd

zoooo0820 reviewed Dec 18, 2023

View reviewed changes

[Add] dtype test cases

1fcf498

zoooo0820 approved these changes Dec 18, 2023

View reviewed changes

luotao1 assigned jeff41404 and sunzhongkai588 Dec 18, 2023

sunzhongkai588 reviewed Dec 19, 2023

View reviewed changes

megemini added 2 commits December 19, 2023 19:27

[Change] start/stop docstring

e89191a

[Change] fix start/stop docstring

7c3de98

megemini requested a review from sunzhongkai588 December 20, 2023 04:33

sunzhongkai588 previously approved these changes Dec 20, 2023

View reviewed changes

jeff41404 reviewed Dec 20, 2023

View reviewed changes

megemini mentioned this pull request Dec 20, 2023

【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API PaddlePaddle/docs#6409

Merged

jeff41404 reviewed Dec 20, 2023

View reviewed changes

[Change] broadcast value to exp_shape

20e1600

megemini dismissed sunzhongkai588’s stale review via 20e1600 December 20, 2023 09:51

megemini requested a review from jeff41404 December 21, 2023 03:56

megemini added 2 commits December 23, 2023 13:20

[Change] axes with list of int

e5fecec

[Add] as tensor test case

8e03291

megemini mentioned this pull request Dec 23, 2023

【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API v2 PaddlePaddle/community#790

Merged

[Change] code style

6b768da

jeff41404 approved these changes Dec 26, 2023

View reviewed changes

luotao1 merged commit c52aec7 into PaddlePaddle:develop Dec 26, 2023

luotao1 changed the title ~~【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API~~ 【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API -part Dec 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API -part #59973

【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API -part #59973

megemini commented Dec 13, 2023 •

edited

Loading

paddle-bot bot commented Dec 13, 2023

megemini commented Dec 18, 2023

zoooo0820 Dec 18, 2023

megemini Dec 18, 2023

zoooo0820 Dec 18, 2023

zoooo0820 Dec 18, 2023

megemini Dec 18, 2023

zoooo0820 left a comment

sunzhongkai588 Dec 19, 2023

megemini Dec 19, 2023

sunzhongkai588 Dec 19, 2023

sunzhongkai588 left a comment

luotao1 commented Dec 20, 2023

jeff41404 Dec 20, 2023

jeff41404 Dec 20, 2023 •

edited

Loading

megemini Dec 20, 2023

jeff41404 Dec 21, 2023 •

edited

Loading

megemini Dec 21, 2023

jeff41404 Dec 22, 2023

jeff41404 Dec 20, 2023 •

edited

Loading

megemini Dec 20, 2023

jeff41404 Dec 20, 2023

megemini commented Dec 20, 2023

megemini commented Dec 21, 2023

megemini commented Dec 24, 2023

jeff41404 commented Dec 26, 2023

jeff41404 left a comment

【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API -part #59973

【Hackathon 5th No.28】为 Paddle 新增 slice_scatter API -part #59973

Conversation

megemini commented Dec 13, 2023 • edited Loading

PR types

PR changes

Description

paddle-bot bot commented Dec 13, 2023

megemini commented Dec 18, 2023

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

zoooo0820 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

sunzhongkai588 left a comment

Choose a reason for hiding this comment

luotao1 commented Dec 20, 2023

Choose a reason for hiding this comment

jeff41404 Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff41404 Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff41404 Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megemini commented Dec 20, 2023

Update 20231220

megemini commented Dec 21, 2023

megemini commented Dec 24, 2023

Update 20231224

jeff41404 commented Dec 26, 2023

jeff41404 left a comment

Choose a reason for hiding this comment

megemini commented Dec 13, 2023 •

edited

Loading

jeff41404 Dec 20, 2023 •

edited

Loading

jeff41404 Dec 21, 2023 •

edited

Loading

jeff41404 Dec 20, 2023 •

edited

Loading