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.

Sign up for GitHub

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

Jump to bottom

API improvement for paddle.argsort and paddle.sort 易用性提升 #63513

Merged
merged 9 commits into from
Apr 23, 2024
Merged

API improvement for paddle.argsort and paddle.sort 易用性提升 #63513

merged 9 commits into from
Apr 23, 2024

Conversation

NKNaN
Copy link
Contributor

@NKNaN NKNaN commented Apr 14, 2024

PR Category

User Experience

PR Types

Improvements

Description

为 paddle.argsort 和 paddle.sort 支持 stable 参数，stable = True 时使用稳定排序算法，stable = False 时不保证排序算法稳定。

@paddle-bot Paddle-bot
Copy link

paddle-bot bot commented Apr 14, 2024

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

@NKNaN
Copy link
Contributor Author

NKNaN commented Apr 14, 2024

针对 sort API: torch返回value和indices，paddle仅返回value 的问题，是否需要新增 return_indices 参数，默认为 False，为 True 时返回 value和indices

@paddle-bot paddle-bot bot added the contributor External developers label Apr 14, 2024
@NKNaN NKNaN changed the title API improvement for paddle.argsort and paddle.sort API improvement for paddle.argsort and paddle.sort 易用性提升 Apr 15, 2024
@NKNaN NKNaN mentioned this pull request Apr 17, 2024
Merged
zhwesky2010
zhwesky2010 reviewed Apr 17, 2024
View reviewed changes
test/legacy_test/test_argsort_op.py Outdated
def init(self):
self.input_shape = [40, 3, 4]
self.axis = 0
self.data = np.array(
Copy link
Contributor

Choose a reason for hiding this comment

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

这个是不是self.input_data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的，已修改

@zhwesky2010
Copy link
Contributor

zhwesky2010 commented Apr 17, 2024 •
edited
Loading

@NKNaN 看一下上面这个self.input_data，单测是不是没有测试到，其他地方没问题了

@NKNaN
Copy link
Contributor Author

NKNaN commented Apr 17, 2024

@NKNaN 看一下上面这个self.input_data，单测是不是没有测试到，其他地方没问题了

好的

zhwesky2010
zhwesky2010 previously approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhwesky2010
Copy link
Contributor

zhwesky2010 commented Apr 17, 2024 •
edited
Loading

@NKNaN custom cpu的kernel修改已经合入了，可以rerun下Py-3，如果还有问题的话，可以本地git clone CustomDevice联调下，看除了custom_kernel:ArgSortKernel的stable参数是否还有其他问题

@zhwesky2010
Copy link
Contributor

@NKNaN op_version.yaml也需要修改一下，参考这个，需要描述对op行为的改变：
infoflow 2024-04-17 17-20-07

https://github.com/PaddlePaddle/community/blob/e71c523c5580cf9755fb29e78755a58e372b5cd8/pfcc/call-for-contributions/paddle_autogen_code_2.md#op_versionyaml%E9%85%8D%E7%BD%AE%E8%AF%B4%E6%98%8E

@NKNaN
Copy link
Contributor Author

NKNaN commented Apr 17, 2024

@NKNaN op_version.yaml也需要修改一下，参考这个，需要描述对op行为的改变

已增加描述。
op_version.yaml 之后在修改 kernel 的时候都需要改吗？

@zhwesky2010 zhwesky2010 self-assigned this Apr 18, 2024
@zhwesky2010
Copy link
Contributor

@NKNaN op_version.yaml也需要修改一下，参考这个，需要描述对op行为的改变

已增加描述。 op_version.yaml 之后在修改 kernel 的时候都需要改吗？

增加/删除input、增加/删除output、增加/删除attr，这几种情况是需要的

@NKNaN
Copy link
Contributor Author

NKNaN commented Apr 18, 2024

增加/删除input、增加/删除output、增加/删除attr，这几种情况是需要的

好的

zhwesky2010
zhwesky2010 approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

LGTM

zyfncg
zyfncg approved these changes Apr 23, 2024
View reviewed changes
paddle/phi/kernels/argsort_kernel.h
Comment on lines +36 to +38
* @param stable Indicate whether to use stable sorting algorithm, which
* guarantees that the order of equivalent elements is
* preserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

注释缩进有点问题

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised here:
#63796

@zhwesky2010
Copy link
Contributor

zhwesky2010 commented Apr 23, 2024 •
edited
Loading

针对 sort API: torch返回value和indices，paddle仅返回value 的问题，是否需要新增 return_indices 参数，默认为 False，为 True 时返回 value和indices

这个当前因为有argsort提供了indices功能，所以先不用修改。

qingqing01
qingqing01 approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@qingqing01 qingqing01 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhwesky2010
Copy link
Contributor

@NKNaN 记得更新一下 API中文文档、API映射文档

@luotao1 luotao1 merged commit e60afe0 into PaddlePaddle:develop Apr 23, 2024
29 of 30 checks passed
@NKNaN
Copy link
Contributor Author

NKNaN commented Apr 23, 2024

这个当前因为有argsort提供了indices功能，所以先不用修改。

好的

@NKNaN
Copy link
Contributor Author

NKNaN commented Apr 23, 2024

@NKNaN 记得更新一下 API中文文档、API映射文档

好的

@NKNaN NKNaN mentioned this pull request Apr 23, 2024
Merged
co63oc pushed a commit to co63oc/Paddle that referenced this pull request Apr 24, 2024
@NKNaN @co63oc
API improvement for paddle.argsort and paddle.sort 易用性提升 (PaddlePaddl…
f9af18e 
…e#63513)

* add stable to argsort and sort

* update

* update xpu kernel and test

* update xpu test

* add argsort docs code example

* fix test

* update op_version.yaml

* fix conflict

* delete additional test
co63oc pushed a commit to co63oc/Paddle that referenced this pull request Apr 25, 2024
@NKNaN @co63oc
API improvement for paddle.argsort and paddle.sort 易用性提升 (PaddlePaddl…
e975328 
…e#63513)

* add stable to argsort and sort

* update

* update xpu kernel and test

* update xpu test

* add argsort docs code example

* fix test

* update op_version.yaml

* fix conflict

* delete additional test
This was referenced Apr 26, 2024
Merged
Merged
@kangguangli kangguangli mentioned this pull request May 27, 2024
Merged
@NKNaN NKNaN mentioned this pull request Jun 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhwesky2010 zhwesky2010 zhwesky2010 approved these changes

@zyfncg zyfncg zyfncg approved these changes

@qingqing01 qingqing01 qingqing01 approved these changes

Assignees

@luotao1 luotao1

@zhwesky2010 zhwesky2010

Labels
contributor External developers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NKNaN @zhwesky2010 @qingqing01 @zyfncg @luotao1