Skip to content

【Hackathon 5th No.34】 为 Paddle 新增 bitwise_right_shift / bitwise_right_shift_ / bitwise_left_shift / bitwise_left_shift_ API #665

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

Merged
merged 15 commits into from
Oct 9, 2023

Conversation

cocoshe
Copy link
Contributor

@cocoshe cocoshe commented Sep 27, 2023

No description provided.

@paddle-bot
Copy link

paddle-bot bot commented Sep 27, 2023

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


## 命名与参数设计

API的设计为`paddle.bitwise_right_shift(x, y)`,其余几个shift操作同理,其中 `x` 与 `y` 需要有相同的shape或者能够进行广播,且类型都必须为int。

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.

如何确定是算术移位还是逻辑移位

辛苦review,需要加参数设定 进行算术位移还是逻辑位移嘛,需要支持的话我修改一下?(因为我调研时看pytorch和numpy中似乎支支持算术位移,所以当时暂时没考虑逻辑位移)

Choose a reason for hiding this comment

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

如果是参数设定,需不需要设置默认参数,默认参数设置为那种,这些内容你可以写到文档里。这次开发算术移位还是逻辑移位是同时开发支持的,jax同时支持两种,你也可以参考下。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

如果是参数设定,需不需要设置默认参数,默认参数设置为那种,这些内容你可以写到文档里。这次开发算术移位还是逻辑移位是同时开发支持的,jax同时支持两种,你也可以参考下。

OK感谢~,现增加了jax中的相关实现,加入is_arithmetic这个bool来确定是算术还是逻辑shift。

Copy link

@xuxinyi389 xuxinyi389 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants