【Hackathon 5th No.6】为 Paddle 增强put_along_axis API -part #59674

YibinLiu666 · 2023-12-04T15:51:44Z

PR types

New features

PR changes

APIs

Description

这个PR的改动为：

按照RFC增强put_along_axis算子，支持min、max、mean规约方式。【Hackathon 5th No.6】为 Paddle 增强put_along_axis API community#636
修复了已有add、mul规约方式梯度计算错误的问题。
为paddle实现了底层的原子乘操作，修复了GPU上乘法计算错误的bug。put_along_axis reduce='mul' 结果不对, cpu正确，gpu错误 #52446
此PR是在fix behavior of put_along_axis and take_along_axis 易用性提升No.43 #59163 基础上进行的修改

paddle-bot · 2023-12-05T02:03:14Z

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

… put_along_axis2

YibinLiu666 · 2023-12-06T02:36:14Z

@zoooo0820 CI都过了，能否麻烦review一下，改动有点大，麻烦您了

zoooo0820 · 2023-12-06T06:26:23Z

paddle/phi/backends/gpu/gpu_primitives.h

+  do {
+    assumed = old;
+    old = atomicCAS(address, assumed, val * assumed);
+  } while (assumed != old);


这里是否也应该有一个返回值

zoooo0820 · 2023-12-07T02:58:07Z

paddle/phi/backends/gpu/gpu_primitives.h

@@ -395,6 +395,181 @@ CUDA_ATOMIC_WRAPPER(Add, complex<double>) {
                         CudaAtomicAdd(imag, val.imag));
 }

+// For atomicMul.


这些atomicMul的计算算法，能提供下参考吗

这里的atomicMul都是参考的前面的atomicAdd以及后面的atomicMin这些，只是把加改成了乘

zoooo0820 · 2023-12-07T07:49:28Z

python/paddle/tensor/manipulation.py

+        ):
+            tensor = paddle.to_tensor(input)
+        else:
+            tensor = input


这组atleast_xxx的改动是否和这个PR无关

应该是之前解决冲突的时候导致的，develop分支的代码是绿色部分，不知道为啥这里显示是我这个PR里面的改动，我再改改

现在没有这个改动了

zoooo0820 · 2023-12-07T07:54:55Z

paddle/phi/kernels/gpu/put_along_axis_kernel.cu

+          *out, axis, index, value, include_self, dev_ctx);
+    } else if (index_type == DataType::INT64) {
+      phi::funcs::gpu_scatter_min_kernel<T, int64_t>(
+          *out, axis, index, value, include_self, dev_ctx);
    }
  } else {
    PADDLE_THROW(errors::InvalidArgument(


报错信息也更新下吧

zoooo0820 · 2023-12-07T08:08:01Z

paddle/phi/kernels/cpu/put_along_axis_kernel.cc

+          *out, axis, index, value, include_self, dev_ctx);
+    } else if (index_type == DataType::INT64) {
+      phi::funcs::cpu_scatter_min_kernel<T, int64_t>(
+          *out, axis, index, value, include_self, dev_ctx);
    }
  } else {
    PADDLE_THROW(errors::InvalidArgument(


报错信息可以更新下

zoooo0820 · 2023-12-07T09:55:27Z

paddle/phi/kernels/funcs/gather_scatter_functor.cc

+  }
+
+  int64_t index_idx = 0;
+  int* num_elements = new int[grad_size]();


此处的num_elements好像没有delete。看起来该项的几处用法都是数组语义，是否可以考虑用stl替代下，更安全一些

已经改成了std::vector

zoooo0820 · 2023-12-07T10:06:17Z

paddle/phi/kernels/funcs/gather_scatter_functor.cc

@@ -268,9 +489,6 @@ void cpu_scatter_value_grad_kernel(phi::DenseTensor self,
    outer_dim_size_grad *= grad_dims[i];
  }
  int64_t index_idx = index.numel() - 1;
-  for (int i = 0; i < grad_size; i++) {
-    grad_data[i] = static_cast<tensor_t>(0);
-  }


想确认下，此处的移除是因为这段赋值是非必要的，还是历史行为是错误的？

这一段是因为value的grad没有初始化为0，我在 #59163 这个PR里面加的，我现在把value grad初始化为0挪到了put_along_axis_grad_kernel.cc以及put_along_axis_grad_kernel.cu初始化这个grad的时候。

zoooo0820 · 2023-12-07T10:21:13Z

paddle/phi/kernels/cpu/put_along_axis_grad_kernel.cc

+            *x_grad,
+            include_self,
+            dev_ctx);
+      } else {


index_type现在在外部有检查吗，这里是只会有int32/int64两种情况吗

之前就是只有这两种情况，我再加个检查

已经在python接口加了类型检查

zoooo0820 · 2023-12-07T10:41:15Z

test/legacy_test/test_put_along_axis_op.py

+        self.axis_type = "int64"
+
+
+class TestPutAlongAxisOpMulIncludeSelf(TestPutAlongAxisOp):


这几个includeself的类测试的是false的情况，命名可以加个not，更清晰一些

zoooo0820 · 2023-12-07T10:42:33Z

test/legacy_test/test_put_along_axis_op.py

+        self.dtype = 'float64'
+        self.x_type = "float64"
+        self.x_shape = (10, 10, 10)
+        self.value_type = "float64"


GPU的场景因为添加了很多dtype的atomicMul的方法，这里能否在单测中补充下目前支持的几个数据类型在新增的reduce方案下的case，保证新增的方案是正确的

由于Optest不支持int类型的输入，所以我统一用的unittest在前向计算做的测试，目前已经加了float32 bfloat16 int32 int64的测试，都没问题

由于Optest不支持int类型的输入，所以我统一用的unittest在前向计算做的测试，目前已经加了float32 bfloat16 int32 int64的测试，都没问题

因为前面代码中包含一个uint8的特殊情况，能再辛苦补充一个uint8类型的测试吗

YibinLiu666 · 2023-12-08T04:41:52Z

@zoooo0820 CI现在都过了，能否辛苦您再review一下，看看还有哪里需要再改改，麻烦您了

zoooo0820 · 2023-12-08T06:39:08Z

paddle/phi/kernels/funcs/gather_scatter_functor.cu

+    phi::CudaAtomicMax(self_data, *src_data);
+  }
+  template <typename tensor_t,
+            std::enable_if_t<std::is_same<tensor_t, uint8_t>::value>* = nullptr>


请问此处是因为uint8没有对应的atomic操作，所以做的特殊处理吗

是的，没有实现uint8的atomic操作，这里的定义我是仿照这个文件之前就定义的reduce_add写的。

zoooo0820 · 2023-12-08T06:45:57Z

test/legacy_test/test_put_along_axis_op.py

+        self.dtype = 'float64'
+        self.x_type = "float64"
+        self.x_shape = (10, 10, 10)
+        self.value_type = "float64"


由于Optest不支持int类型的输入，所以我统一用的unittest在前向计算做的测试，目前已经加了float32 bfloat16 int32 int64的测试，都没问题

因为前面代码中包含一个uint8的特殊情况，能再辛苦补充一个uint8类型的测试吗

zoooo0820

LGTM

vivienfanghuagood

LGTM for api change

jeff41404 · 2023-12-12T04:30:09Z

paddle/phi/api/yaml/ops.yaml

@@ -2035,7 +2035,7 @@
  backward : psroi_pool_grad

 - op : put_along_axis
-  args : (Tensor arr, Tensor indices, Tensor values, int axis, str reduce = "assign")
+  args : (Tensor arr, Tensor indices, Tensor values, int axis, str reduce = "assign", bool include_self = true)


there is a parameter of broadcast in Python API, shall we also add it here as include_self or delete it from API？

broadcast is processed in the Python interface, so there is no need to pass it into the C interface again

jeff41404 · 2023-12-12T04:32:33Z

The docstring needs to be modified, such as missing an introduction to the parameter of values, and the example code is too simple, requiring the addition of example code for multiple usage methods. also modifying Chinese documents.

YibinLiu666 · 2023-12-12T05:05:02Z

The docstring needs to be modified, such as missing an introduction to the parameter of values, and the example code is too simple, requiring the addition of example code for multiple usage methods. also modifying Chinese documents.

Done for doc of en. Chinese doc is modified at PaddlePaddle/docs#6348

jeff41404

LGTM

sunzhongkai588

LGTM
一些typo小问题，新提一个 PR 修改吧 @YibinLiu666

sunzhongkai588 · 2023-12-13T08:03:15Z

python/paddle/tensor/manipulation.py

-        reduce (str, optional): The reduce operation, default is 'assign', support 'add', 'assign', 'mul' and 'multiply'.
-        include_self (bool, optional): whether to reduce with the elements of arr. (Only support True now)
-        broadcast (bool, optional): whether to broadcast indices.
+        reduce (str, optional): The reduce operation, default is 'assign', support 'add', 'assign', 'mul', 'multiply', "mean", "amin" and "amax".


Suggested change

reduce (str, optional): The reduce operation, default is 'assign', support 'add', 'assign', 'mul', 'multiply', "mean", "amin" and "amax".

reduce (str, optional): The reduce operation, default is 'assign', support 'add', 'assign', 'mul', 'multiply', 'mean', 'amin' and 'amax'.

和前文统一吧

…e#59674)

* 【Hackathon 5th No.6】为 Paddle 增强put_along_axis API -part (#59674) * fix bug of put_along_axis (#60551) * Improve the performence of put_along_axis (#60618) * fix bug of put_along_axis * improve performence of put_along_axis * [Bug-Fix] fix compile bug of cudaxxxAsync (#60934) --------- Co-authored-by: YibLiu <68105073+YibinLiu666@users.noreply.github.com>

YibinLiu666 added 15 commits November 20, 2023 08:50

fix behavior of put_along_axis and take_along_axis

b6cedc3

fix error

36a2405

fix take_along_axis used in stat

3818298

update

18dc8c3

fix build error

61461f4

add test for error

013dfb4

add param broadcast

1155d4c

use origin example

efc488c

add param include_self

675b641

update param name

2c968e3

modify ut

d32db6f

update test case

564de93

add error UT

d0c14de

update

28aadd6

strength put_along_axis

c2fdb6f

YibinLiu666 force-pushed the put_along_axis2 branch from 69b10d3 to c2fdb6f Compare December 4, 2023 16:46

Merge branch 'develop' into put_along_axis2

d3e33a2

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

luotao1 added the PaddlePaddle Hackathon label Dec 5, 2023

luotao1 assigned luotao1 and zoooo0820 Dec 5, 2023

luotao1 mentioned this pull request Dec 5, 2023

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

Open

YibinLiu666 and others added 4 commits December 5, 2023 11:20

Update gather_scatter_functor.h

1903b91

Update gather_scatter_functor.cu

5a88f79

Update manipulation.py

8f03013

fix codestyle

d215bf8

YibinLiu666 mentioned this pull request Dec 5, 2023

【Hackathon 5th No.6】为 Paddle 增强put_along_axis API PaddlePaddle/docs#6348

Merged

YibinLiu666 added 2 commits December 5, 2023 08:09

rebase

8c13743

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

a6b7d16

… put_along_axis2

zoooo0820 reviewed Dec 7, 2023

View reviewed changes

update

db3168f

YibinLiu666 mentioned this pull request Dec 8, 2023

[WeeklyReports] 2023.11.26~2023.12.10 周报收集 PFCCLab/Starter#37

Closed

26 tasks

zoooo0820 reviewed Dec 8, 2023

View reviewed changes

add test for uint8

2525f2a

zoooo0820 approved these changes Dec 11, 2023

View reviewed changes

vivienfanghuagood approved these changes Dec 11, 2023

View reviewed changes

luotao1 assigned jeff41404 and sunzhongkai588 Dec 11, 2023

jeff41404 reviewed Dec 12, 2023

View reviewed changes

update doc

7709066

jeff41404 approved these changes Dec 13, 2023

View reviewed changes

sunzhongkai588 approved these changes Dec 13, 2023

View reviewed changes

luotao1 changed the title ~~【Hackathon 5th No.6】为 Paddle 增强put_along_axis API~~ 【Hackathon 5th No.6】为 Paddle 增强put_along_axis API -part Dec 13, 2023

luotao1 merged commit c35c63e into PaddlePaddle:develop Dec 13, 2023
29 checks passed

YibinLiu666 mentioned this pull request Dec 13, 2023

【Hackathon 5th No.6】英文 doc 文档修复 -part #59985

Merged

YibinLiu666 deleted the put_along_axis2 branch December 13, 2023 08:52

warrentdrew pushed a commit to warrentdrew/Paddle that referenced this pull request Feb 5, 2024

【Hackathon 5th No.6】为 Paddle 增强put_along_axis API -part (PaddlePaddl…

eefc79d

…e#59674)

warrentdrew mentioned this pull request Feb 5, 2024

[cherry-pick] add commits for fixing put_along_axis #61612

Closed

zhwesky2010 pushed a commit to zhwesky2010/Paddle that referenced this pull request Feb 26, 2024

【Hackathon 5th No.6】为 Paddle 增强put_along_axis API -part (PaddlePaddl…

6119ca6

…e#59674)

zhwesky2010 mentioned this pull request Feb 26, 2024

[cherry-pick 2.6] Fix bug of put_along_axis/take_along_axis #62065

Merged

		self.axis_type = "int64"


		class TestPutAlongAxisOpMulIncludeSelf(TestPutAlongAxisOp):

	reduce (str, optional): The reduce operation, default is 'assign', support 'add', 'assign', 'mul', 'multiply', "mean", "amin" and "amax".
	reduce (str, optional): The reduce operation, default is 'assign', support 'add', 'assign', 'mul', 'multiply', 'mean', 'amin' and 'amax'.

【Hackathon 5th No.6】 为 Paddle 增强put_along_axis API -part #59674

【Hackathon 5th No.6】 为 Paddle 增强put_along_axis API -part #59674

Conversation

YibinLiu666 commented Dec 4, 2023 • edited Loading

PR types

PR changes

Description

paddle-bot bot commented Dec 5, 2023

YibinLiu666 commented Dec 6, 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

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

YibinLiu666 Dec 7, 2023 • 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

YibinLiu666 commented Dec 8, 2023

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

vivienfanghuagood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff41404 commented Dec 12, 2023

YibinLiu666 commented Dec 12, 2023 • edited Loading

jeff41404 left a comment

Choose a reason for hiding this comment

sunzhongkai588 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

【Hackathon 5th No.6】为 Paddle 增强put_along_axis API -part #59674

【Hackathon 5th No.6】为 Paddle 增强put_along_axis API -part #59674

YibinLiu666 commented Dec 4, 2023 •

edited

Loading

YibinLiu666 Dec 7, 2023 •

edited

Loading

YibinLiu666 commented Dec 12, 2023 •

edited

Loading

sunzhongkai588 left a comment •

edited

Loading