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

Hackathon No.16】add RFC for take API #217

Merged
merged 17 commits into from Aug 29, 2022
Merged

Conversation

S-HuaBomb
Copy link
Contributor

@S-HuaBomb S-HuaBomb commented Aug 22, 2022

为 paddle 新增 take API。

根据 PaddlePaddle/community#186 (review) 重新修改了本 RFC 和 PR PaddlePaddle/Paddle#44741

@paddle-bot
Copy link

paddle-bot bot commented Aug 22, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请检查PR提交格式和内容是否完备,具体请参考示例模版
Your PR has been submitted. Thanks for your contribution!
Please check its format and content. For this, you can refer to Template and Demo.

2. 根据 mode 参数对索引进行越界处理:
- `mode='raise'`, 直接抛出错误 (默认);
- `mode='wrap'`, 通过取余约束越界的 indices;
- `mode='clip'`, 通过 `paddle.clip` 约束两端的索引。
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. 需要在业内方案调研的Numpy实现方法里,补充下mode参数的越界处理的逻辑和核心代码
  2. raise抛出错误,不是在mode参数阶段抛出的,是在后面的index_select地方抛出,这里的描述不够准确
  3. 需要说明下wrap是用哪个API来完成处理的。
  4. 因为Clip: Note that this disables indexing with negative numbers.有这个限制,需要说明下直接使用paddle.clip是否符合要求

@S-HuaBomb
Copy link
Contributor Author

Reply: #217 (comment)

RFC 增加了 numpy.take 的 mode 参数的核心代码以及总结。

  • paddle.take 索引越界的三种处理方式:
    • mode='raise',若索引越界,通过最后调用的 paddle.index_select 抛出错误 (默认);
    • mode='wrap',通过取余约束越界的 indices,不需调用 API;
    • mode='clip',通过 paddle.clip 将两端超出范围的索引约束到 [0, max_index-1]。

Copy link
Collaborator

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1 luotao1 merged commit 8e9a794 into PaddlePaddle:master Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants