【Hackathon 5th No.6】为 Paddle 增强put_along_axis API -part by YibinLiu666 · Pull Request #59674 · PaddlePaddle/Paddle

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

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


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

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

                         CudaAtomicAdd(imag, val.imag));
 }

+// For atomicMul.


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

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

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

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


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

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

现在没有这个改动了

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

+          *out, axis, index, value, include_self, dev_ctx);
    }
  } else {
    PADDLE_THROW(errors::InvalidArgument(


报错信息也更新下吧

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

+          *out, axis, index, value, include_self, dev_ctx);
    }
  } else {
    PADDLE_THROW(errors::InvalidArgument(


报错信息可以更新下

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

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


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

已经改成了std::vector

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

  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

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


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

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

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

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

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


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

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

+        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

+    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

+        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


 - 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

-        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 <[email protected]>

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

Closed

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

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

Enigmatisms mentioned this pull request Sep 7, 2025

[PHI] Fix fp16/int16 atomic primitives #75142

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'.

Conversation

YibinLiu666 commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR types

PR changes

Description

Uh oh!

paddle-bot Bot commented Dec 5, 2023

Uh oh!

YibinLiu666 commented Dec 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YibinLiu666 Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YibinLiu666 commented Dec 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoooo0820 left a comment

Choose a reason for hiding this comment

Uh oh!

vivienfanghuagood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeff41404 commented Dec 12, 2023

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