-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 5th No.37】为 Paddle 新增 householder_product API -part #58214
【Hackathon 5th No.37】为 Paddle 新增 householder_product API -part #58214
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
def test_error(self): | ||
with self.assertRaises(AssertionError): | ||
x = paddle.randn([3, 2, 1], dtype=paddle.float32) | ||
tau = paddle.randn([6, 5, 4], dtype=paddle.float64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果只是判断shape dismatch,是否需要控制dtype相同
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已修改,辛苦review~
def test_error(self): | ||
with self.assertRaises(AssertionError): | ||
x = paddle.randn([3, 6], dtype=paddle.float32) | ||
tau = paddle.randn([6], dtype=paddle.float64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同上
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已修改,辛苦review~
CodeStyle和Windows-inference流水线没有过 |
python/paddle/tensor/linalg.py
Outdated
"and the dimension of A is 1 larger than the dimension of tau\n" | ||
) | ||
assert ( | ||
A.shape[-2] >= A.shape[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里是保证A矩阵的m >= n吗,tau矩阵是否要保证 n >= k?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
householder_product当前实现方案支持了实值,torch中有对复数的支持吗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
辛苦review~
这里是保证A矩阵的m >= n吗,tau矩阵是否要保证 n >= k?
是的,这里应该在加一些assert和对应的单测,我稍后补上,限制条件发现了一些小bug
householder_product当前实现方案支持了实值,torch中有对复数的支持吗?
是的,我当时直接用了paddle.norm(它暂时没支持复数实现),不过应该可以直接手动实现一下norm,我稍后试下再补上相应单测~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好的
python/paddle/tensor/linalg.py
Outdated
A.shape[idx] == tau.shape[idx] | ||
), "The input A must have the same batch dimensions with input tau.\n" | ||
|
||
def _norm(x): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个函数有用到吗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个函数有用到吗
噢这个_norm应该用不到,我稍后删掉(本来用来解决complex类型时候的norm用)
话说像现在这个ci是我这边的啥问题嘛?今天早上好像就改了下codestyle
https://xly.bce.baidu.com/paddlepaddle/paddle/newipipe/detail/9536759/job/24450158
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个不是你的问题,是那个单测有一些问题
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://xly.bce.baidu.com/paddlepaddle/paddle/newipipe/detail/9538644/job/24453315
好怪为啥我的pre-commit本地可以pass,但是ci里面却报错
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cocoshe 昨天isort更新版本了,可以拉一下最新代码,然后重新precommit一下
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cocoshe 昨天isort更新版本了,可以拉一下最新代码,然后重新precommit一下
okk~
… householder_product_coco_dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
python/paddle/tensor/linalg.py
Outdated
@@ -3724,3 +3724,133 @@ def cdist( | |||
return paddle.linalg.norm( | |||
x[..., None, :] - y[..., None, :, :], p=p, axis=-1 | |||
) | |||
|
|||
|
|||
def householder_product(A, tau, name=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to API naming conventions, enter the name of Tensor using x
, and the rfc should also be modified synchronously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
请提交中文文档PR |
Co-authored-by: zachary sun <[email protected]>
Co-authored-by: zachary sun <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM~
预览链接暂时挂了,辛苦在合入的第二天,直接去官网API文档的 develop 分支里看下,是否有格式问题~(肉眼看暂时没问题) @cocoshe
ok我到时候确认一下,之前自己本地vscode插件渲染看过一下 |
PR types
New features
PR changes
APIs
Description
RFC:
文档: