Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

【PaddlePaddle Hackathon 2】11新增 API index_fill #42454

Closed
wants to merge 9 commits into from

Conversation

thunder95
Copy link
Contributor

PR types

New features

PR changes

APIs

Describe

完成第二期第11项目开发任务: #40324
对于 nd tensor, 沿着某个轴 axis 取 (n-1)d 的切片,索引位置是 index, 并且将 value 中值填充到这些切片上。其中 value 是一个 scalar 或者 0d tensor

RFC设计文档: PaddlePaddle/community#122

@paddle-bot-old
Copy link

paddle-bot-old bot commented May 3, 2022

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

fill_value)

helper = LayerHelper("index_fill", **locals())
check_variable_and_dtype(x, 'x', ['float32', 'float64', 'int32', 'int64'],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增加注册的数据类型后,此处需要同步修改。

@thunder95
Copy link
Contributor Author

thunder95 commented May 14, 2022

@iclementine 老师,您好,似乎我没有用AxisTensor以及IndexTensor,也可以运行并测试成功,AxisTensor和axis这种组合是必须的吗?

如果按照slice的那种写法,就会报错, 不清楚为什么slice可以。动态图里scale里面也没有将scale时tensor时作为输入,只是经态图有相应的改变。

868: Traceback (most recent call last):
868: File "/paddle/build/python/paddle/fluid/tests/unittests/test_index_fill_op.py", line 307, in test_check_grad
868: x_tensor, index=idx_tensor, axis=self.axis, fill_value=0.5)
868: File "/paddle/build/python/paddle/tensor/manipulation.py", line 4337, in index_fill
868: return _C_ops.index_fill(x, index_tensor, axis_tensor, *attrs)
868: ValueError: (InvalidArgument) index_fill(): argument (position 1) must be str, but got Tensor (at /paddle/paddle/fluid/pybind/op_function_common.cc:661)

静态图按照slice的写法是没有问题的。

@OccupyMars2025
Copy link
Contributor

Screenshot from 2022-05-16 10-17-07

@OccupyMars2025
Copy link
Contributor

打扰了,我是参赛者,我觉得上面的代码可能有问题,如果没问题,那当我没说。是否需要在使用EigenTensor<T, 3>之前,将x 和 fill_grad 先 Resize 成 3 维的 Tensor, 因为你在 if(output) { 中也是这样做的, 但是 x 是 const的, 无法 Resize

@thunder95
Copy link
Contributor Author

打扰了,我是参赛者,我觉得上面的代码可能有问题,如果没问题,那当我没说。是否需要在使用EigenTensor<T, 3>之前,将x 和 fill_grad 先 Resize 成 3 维的 Tensor, 因为你在 if(output) { 中也是这样做的, 但是 x 是 const的, 无法 Resize

@SmirnovKol 嗯 严谨一些,需要resize,待审核老师回复可以一起优化一下。

@thunder95
Copy link
Contributor Author

@SmirnovKol 因为这里,在你的代码里测试出bug了吗?

@OccupyMars2025
Copy link
Contributor

关于这个我没有跑出bug, 我只是单纯地有点看不懂,谢谢回复

@OccupyMars2025
Copy link
Contributor

Screenshot from 2022-05-16 10-59-38

@OccupyMars2025
Copy link
Contributor

关于这个函数def index_fill_fill_value_grad_np(data, axis, index):
这个函数的作用似乎只是统计了 data中被index 取到的所有元素的个数,如果是那样,可以写简单一点,比如:
shape = data.shape
shape[axis] = len(index)
return int(np.prod(shape))

@OccupyMars2025
Copy link
Contributor

index_fill 中的 fill_value, index_add中的 add_value:
如果两者都是只有1个元素的tensor, 求其grad 应该怎么求 ??

你的思路似乎是 将 X 中所有被 index 取到的 元素的个数作为 fill_value 的 grad,
但是我觉得 将 out_grad 中所有被 index 取到的 元素之和 作为 fill_value的 grad 也许会更好,
同样我觉得 将 out_grad 中所有被 index 取到的 元素之和 作为 add_value 的 grad 比较合适(这里我假设 index_add中的index 不包含重复的元素),

@thunder95
Copy link
Contributor Author

关于这个函数def index_fill_fill_value_grad_np(data, axis, index): 这个函数的作用似乎只是统计了 data中被index 取到的所有元素的个数,如果是那样,可以写简单一点,比如: shape = data.shape shape[axis] = len(index) return int(np.prod(shape))

@SmirnovKol 说的没错,是可以这么简化计算。当时主要是为了体现在算子里的计算逻辑,方便后面好回忆。

@thunder95
Copy link
Contributor Author

index_fill 中的 fill_value, index_add中的 add_value: 如果两者都是只有1个元素的tensor, 求其grad 应该怎么求 ??

你的思路似乎是 将 X 中所有被 index 取到的 元素的个数作为 fill_value 的 grad, 但是我觉得 将 out_grad 中所有被 index 取到的 元素之和 作为 fill_value的 grad 也许会更好, 同样我觉得 将 out_grad 中所有被 index 取到的 元素之和 作为 add_value 的 grad 比较合适(这里我假设 index_add中的index 不包含重复的元素),

@SmirnovKol 那个地方代码组织得有些不太好,命名容易误解,当时也是为了抽取共同的代码,减少代码量。实际上,可以仔细阅读下,fill_value取的也是初始的out_grad梯度之和

@OccupyMars2025
Copy link
Contributor

OccupyMars2025 commented May 16, 2022

我还想确认一下:

  1. paddle.index_fill 和 paddle.index_add 中的 index 能否包含重复的元素, 比如 [1, 2, 2],
    如果包含重复的元素, 按照我的理解,在目前的gpu forward kernel中, 可能会有几个并发线程同时处理1个元素的情况,会有冲突 (我对CUDA理解不深, 如果没有这问题,那就算了)

  2. 现在的 paddle.index_fill 中的 fill_value 和 paddle.index_add 中的 add_value 都只支持 只含有1个元素的tensor, 是否需要支持任意shape( 只要 该shape 能 broadcast 到 切片后的形状) (如何实现,paddle专家能否给一些提示)???

@OccupyMars2025
Copy link
Contributor

OccupyMars2025 commented May 16, 2022

我仔细看了下,这个函数还是只是统计了被index取到的元素的个数, 而你在单侧中直接用该函数求 fill_value的grad
Screenshot from 2022-05-16 11-28-49

@OccupyMars2025
Copy link
Contributor

我借用一下你的地方提问题。请 paddle专家解答一下吧。

@OccupyMars2025
Copy link
Contributor

我仔细看了下,这个函数还是只是统计了被index取到的元素的个数, 而你在单侧中直接用该函数求 fill_value的grad Screenshot from 2022-05-16 11-28-49

对,你在paddle中求fill_value的grad是用的out_grad中被index到的元素之和, 我被def index_fill_fill_value_grad_np(data, axis, index): 的计算逻辑误导了,这个函数在单测中使用的背景情况是:out_grad为元素值全为1的tensor, 所以你使用才没问题。好像单侧时求grad, 所有out_grad 都是 元素值全为1的tensor, 如何测试out_grad为任意tensor,求 x_grad, fill_value_grad ? 我再研究一下

@thunder95
Copy link
Contributor Author

我仔细看了下,这个函数还是只是统计了被index取到的元素的个数, 而你在单侧中直接用该函数求 fill_value的grad Screenshot from 2022-05-16 11-28-49

对,你在paddle中求fill_value的grad是用的out_grad中被index到的元素之和, 我被def index_fill_fill_value_grad_np(data, axis, index): 的计算逻辑误导了,这个函数在单测中使用的背景情况是:out_grad为元素值全为1的tensor, 所以你使用才没问题。好像单侧时求grad, 所有out_grad 都是 元素值全为1的tensor, 如何测试out_grad为任意tensor,求 x_grad, fill_value_grad ? 我再研究一下

好的 欢迎大佬批评指正

@thunder95
Copy link
Contributor Author

我还想确认一下:

1. paddle.index_fill 和  paddle.index_add 中的 index 能否包含重复的元素, 比如 [1, 2, 2],
   如果包含重复的元素, 按照我的理解,在目前的gpu  forward  kernel中, 可能会有几个并发线程同时处理1个元素的情况,会有冲突 (我对CUDA理解不深, 如果没有这问题,那就算了)

2. 现在的 paddle.index_fill 中的 fill_value  和  paddle.index_add 中的 add_value 都只支持 只含有1个元素的tensor,  是否需要支持任意shape( 只要 该shape 能 broadcast 到 切片后的形状) (如何实现,paddle专家能否给一些提示)???

对于第一点, index_fill只是覆盖原来的元素,所以问题不大。
对于第二点,index_fill应该没有这个需求,只是填充常量。

@OccupyMars2025
Copy link
Contributor

OccupyMars2025 commented May 16, 2022

你说得没错,index_fill 应该问题不大, 我自己再研究一下吧

@iclementine
Copy link

iclementine commented May 17, 2022

@iclementine 老师,您好,似乎我没有用AxisTensor以及IndexTensor,也可以运行并测试成功,AxisTensor和axis这种组合是必须的吗?

如果按照slice的那种写法,就会报错, 不清楚为什么slice可以。动态图里scale里面也没有将scale时tensor时作为输入,只是经态图有相应的改变。

868: Traceback (most recent call last): 868: File "/paddle/build/python/paddle/fluid/tests/unittests/test_index_fill_op.py", line 307, in test_check_grad 868: x_tensor, index=idx_tensor, axis=self.axis, fill_value=0.5) 868: File "/paddle/build/python/paddle/tensor/manipulation.py", line 4337, in index_fill 868: return _C_ops.index_fill(x, index_tensor, axis_tensor, *attrs) 868: ValueError: (InvalidArgument) index_fill(): argument (position 1) must be str, but got Tensor (at /paddle/paddle/fluid/pybind/op_function_common.cc:661)

静态图按照slice的写法是没有问题的。

你指的是,不需要在 kernel 中将 axis 和 index 两个参数声明为 Scalar 和 IntArray, 也能在 python API 中传入 Tensor 吗?

@thunder95
Copy link
Contributor Author

@iclementine 老师,您好,似乎我没有用AxisTensor以及IndexTensor,也可以运行并测试成功,AxisTensor和axis这种组合是必须的吗?
如果按照slice的那种写法,就会报错, 不清楚为什么slice可以。动态图里scale里面也没有将scale时tensor时作为输入,只是经态图有相应的改变。
868: Traceback (most recent call last): 868: File "/paddle/build/python/paddle/fluid/tests/unittests/test_index_fill_op.py", line 307, in test_check_grad 868: x_tensor, index=idx_tensor, axis=self.axis, fill_value=0.5) 868: File "/paddle/build/python/paddle/tensor/manipulation.py", line 4337, in index_fill 868: return _C_ops.index_fill(x, index_tensor, axis_tensor, *attrs) 868: ValueError: (InvalidArgument) index_fill(): argument (position 1) must be str, but got Tensor (at /paddle/paddle/fluid/pybind/op_function_common.cc:661)
静态图按照slice的写法是没有问题的。

你指的是,不需要在 kernel 中将 axis 和 index 两个参数声明为 Scalar 和 IntArray, 也能在 python API 中传入 Tensor 吗?

需要在kernel里声明 Scalar 和 IntArray。 我指的是在python声明的函数里python/paddle/tensor/manipulation.py, 动态图模式下index和axis都是tensor,例如L4317, 现在写成return _C_ops.index_fill(x, "index", index, "axis", _axis, "fill_value", fill_value)就可以直接兼容scalar和tensor类型,不需要写成return _C_ops.index_fill(x, IndexTensor, AxisTensor, "fill_value", fill_value)的方式,如果按后者的写法反而会报错。 @iclementine

@thunder95
Copy link
Contributor Author

@iclementine 虽然这次任务失败,希望老师可以继续review,我想完成这个PR

@iclementine
Copy link

@iclementine 虽然这次任务失败,希望老师可以继续review,我想完成这个PR

好的呀,谢谢支持哦~,我们继续交流。

@iclementine
Copy link

iclementine commented May 17, 2022

@iclementine 老师,您好,似乎我没有用AxisTensor以及IndexTensor,也可以运行并测试成功,AxisTensor和axis这种组合是必须的吗?
如果按照slice的那种写法,就会报错, 不清楚为什么slice可以。动态图里scale里面也没有将scale时tensor时作为输入,只是静态图有相应的改变。
868: Traceback (most recent call last): 868: File "/paddle/build/python/paddle/fluid/tests/unittests/test_index_fill_op.py", line 307, in test_check_grad 868: x_tensor, index=idx_tensor, axis=self.axis, fill_value=0.5) 868: File "/paddle/build/python/paddle/tensor/manipulation.py", line 4337, in index_fill 868: return _C_ops.index_fill(x, index_tensor, axis_tensor, *attrs) 868: ValueError: (InvalidArgument) index_fill(): argument (position 1) must be str, but got Tensor (at /paddle/paddle/fluid/pybind/op_function_common.cc:661)
静态图按照slice的写法是没有问题的。

你指的是,不需要在 kernel 中将 axis 和 index 两个参数声明为 Scalar 和 IntArray, 也能在 python API 中传入 Tensor 吗?

需要在kernel里声明 Scalar 和 IntArray。 我指的是在python声明的函数里python/paddle/tensor/manipulation.py, 动态图模式下index和axis都是tensor,例如L4317, 现在写成return _C_ops.index_fill(x, "index", index, "axis", _axis, "fill_value", fill_value)就可以直接兼容scalar和tensor类型,不需要写成return _C_ops.index_fill(x, IndexTensor, AxisTensor, "fill_value", fill_value)的方式,如果按后者的写法反而会报错。 @iclementine

sclae 使用了比较 tricky 的方式来处理

    if in_dygraph_mode():
        # 这个是近期的动态图方案,是间接调用 c++ API 实现的
        out = _C_ops.final_state_scale(x, scale, float(bias), bias_after_scale)
        return dygraph_utils._append_activation_in_dygraph(out)
    if _non_static_mode():
        # 这里利用了动态图下可以对 Tensor 求值的特征,如果 scale 是 Tensor, 就把 scale 转换成了 float
        _scale = scale.numpy().item(0) if isinstance(scale, Variable) else scale
        out = _C_ops.scale(x, 'scale',
                           float(_scale), 'bias',
                           float(bias), 'bias_after_scale', bias_after_scale)
        return dygraph_utils._append_activation_in_dygraph(out)

至于如果用写成return _C_ops.index_fill(x, IndexTensor, AxisTensor, "fill_value", fill_value)的方式,反而报错的原因,这个看起来是和 operator 的签名有关了。它的处理方式是

op_function_name(input1, input2,...,  attr_name1, attr_value1, attr_name2, attr_value2,...)

这样传参的。Input 都是直接传入的,attr 都是以 (name, value) 的 pair 展平的方式传入的,用来模拟关键字传参的方式。其中从哪个参数开始算是 attr 是由 opmaker 来决定的。比如 opmake 中定义是有三个 Input,那么从第四个开始才算是 attr. 目前看起来是你的 opmaker 只是定义了一个 Input,所以调用的时候认为第二个参数开始就算是 attr, 所以它认为这个参数是一个 attr_name, 需要是字符串。

我需要 pull 一下你的代码做做测试才能给出明确的答案。

@iclementine
Copy link

iclementine commented May 17, 2022

我还想确认一下:

1. paddle.index_fill 和  paddle.index_add 中的 index 能否包含重复的元素, 比如 [1, 2, 2],
   如果包含重复的元素, 按照我的理解,在目前的gpu  forward  kernel中, 可能会有几个并发线程同时处理1个元素的情况,会有冲突 (我对CUDA理解不深, 如果没有这问题,那就算了)

2. 现在的 paddle.index_fill 中的 fill_value  和  paddle.index_add 中的 add_value 都只支持 只含有1个元素的tensor,  是否需要支持任意shape( 只要 该shape 能 broadcast 到 切片后的形状) (如何实现,paddle专家能否给一些提示)???

paddle 里面有 broadcast 的机制,可以参考 elementwise 系列 op 的实现机制。正向的时候进行了 broadcast, 反向的时候就会有一个相应的 reduce. 可以参考一下 paddle/phi/kernels/elementwise_kernel.h. 但是可能 elementwise 系列的还稍微简单一些, Index_add 如果要实现 broadcast 可能比这个还要复杂一些,因为 index 切片可能不是连续的。

@OccupyMars2025
Copy link
Contributor

OccupyMars2025 commented May 18, 2022

@iclementine感谢指导。我学习heaviside op时接触到了elementwise 系列op, 当时就猜想要想add_value 能是任意shape, 可能要借鉴使用elementwise op的实现。奈何肚子里墨水少,只学过几句话,还写不了大文章,等我学完背完十几、二十几篇大文章后,我应该就能独立完成一篇文章了。读书破万卷,下笔如有神。现在觉得这句话真是不变的真理。

@paddle-bot-old
Copy link

Sorry to inform you that 61e514e's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

@paddle-bot
Copy link

paddle-bot bot commented Sep 5, 2023

Since you haven't replied for more than a year, we have closed this issue/pr.
If the problem is not solved or there is a follow-up one, please reopen it at any time and we will continue to follow up.
由于您超过一年未回复,我们将关闭这个issue/pr。
若问题未解决或有后续问题,请随时重新打开,我们会继续跟进。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants