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.2】为 Paddle 新增 cdist API #53836

Merged
merged 22 commits into from
Jun 2, 2023
Merged

【Hackathon No.2】为 Paddle 新增 cdist API #53836

merged 22 commits into from
Jun 2, 2023

Conversation

GreatV
Copy link
Contributor

@GreatV GreatV commented May 16, 2023

PR types

New features

PR changes

APIs

Description

Add cdist op

@paddle-bot
Copy link

paddle-bot bot commented May 16, 2023

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

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels May 16, 2023
@paddle-bot
Copy link

paddle-bot bot commented May 16, 2023

❌ The PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

@GreatV GreatV changed the title 【Hackathon + No.2】为 Paddle 新增 cdist API 【Hackathon No.2】为 Paddle 新增 cdist API May 16, 2023
@luotao1 luotao1 added the API label May 16, 2023
@GGBond8488
Copy link
Contributor

Hello, 对于非必要新增op的API ,目前我们不倾向新增OP,最好以组合的形式实现该API

@GreatV
Copy link
Contributor Author

GreatV commented May 16, 2023

Hello, 对于非必要新增op的API ,目前我们不倾向新增OP,最好以组合的形式实现该API

只用python端实现类似这样的效果,就可以了吗

import paddle

def _cdist(x : paddle.Tensor, y : paddle.Tensor, p=2, name=None):
    r1 = x.shape[-2]
    r2 = y.shape[-2]
    if r1 == 0 or r2 == 0:
        return paddle.empty((r1, r2), dtype=x.dtype, name=name)
    
    return paddle.norm(x[..., None, :] - y[..., None, :, :], p=p, axis=-1, name=name)

@GGBond8488
Copy link
Contributor

Hello, 对于非必要新增op的API ,目前我们不倾向新增OP,最好以组合的形式实现该API

只用python端实现类似这样的效果,就可以了吗

import paddle

def _cdist(x : paddle.Tensor, y : paddle.Tensor, p=2, name=None):
    r1 = x.shape[-2]
    r2 = y.shape[-2]
    if r1 == 0 or r2 == 0:
        return paddle.empty((r1, r2), dtype=x.dtype, name=name)
    
    return paddle.norm(x[..., None, :] - y[..., None, :, :], p=p, axis=-1, name=name)

对的

python/paddle/tensor/linalg.py Outdated Show resolved Hide resolved
python/paddle/tensor/linalg.py Outdated Show resolved Hide resolved
python/paddle/fluid/tests/unittests/test_cdist_op.py Outdated Show resolved Hide resolved
python/paddle/tensor/linalg.py Show resolved Hide resolved
@GreatV GreatV requested a review from GGBond8488 May 19, 2023 01:36
GGBond8488
GGBond8488 previously approved these changes May 30, 2023
Copy link
Contributor

@GGBond8488 GGBond8488 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sunzhongkai588 sunzhongkai588 left a 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 Show resolved Hide resolved
python/paddle/tensor/linalg.py Outdated Show resolved Hide resolved
python/paddle/tensor/linalg.py Outdated Show resolved Hide resolved
GreatV and others added 2 commits May 31, 2023 12:18
Co-authored-by: zachary sun <70642955+sunzhongkai588@users.noreply.github.com>
@GreatV
Copy link
Contributor Author

GreatV commented May 31, 2023

辛苦修改~同时请提供中文文档

好的,下个PR,提供中文文档。

@jeff41404
Copy link
Contributor

Some descriptions in rfc need to be modified:
chapter1,“功能目标” 中 “为 Paddle 新增 cdist API。dist API 用于计算两个输入 Tensor 的 p 范数(p-norm),计算结果为形状为 [1] 的 Tensor,而 cdist API 则用于计算两个输入 Tensor 的所有行向量对的 p 范数(p-norm),输出结果的形状和两个 Tensor 乘积的形状一致。” we have corrected the semantics of the 0-dimensional tensor in the develop version, and the shape of output of dist is [], not [1].
chapter5,“命名与参数设计“ 中”API设计为 paddle.cdist(x1, x2, p=2.0, compute_mode='use_mm_for_euclid_dist_if_necessary')” is different from the parameters in the code: paddle.cdist(x, y, p=2.0, compute_mode='use_mm_for_euclid_dist_if_necessary')”

@GreatV
Copy link
Contributor Author

GreatV commented May 31, 2023

Some descriptions in rfc need to be modified: chapter1,“功能目标” 中 “为 Paddle 新增 cdist API。dist API 用于计算两个输入 Tensor 的 p 范数(p-norm),计算结果为形状为 [1] 的 Tensor,而 cdist API 则用于计算两个输入 Tensor 的所有行向量对的 p 范数(p-norm),输出结果的形状和两个 Tensor 乘积的形状一致。” we have corrected the semantics of the 0-dimensional tensor in the develop version, and the shape of output of dist is [], not [1]. chapter5,“命名与参数设计“ 中”API设计为 paddle.cdist(x1, x2, p=2.0, compute_mode='use_mm_for_euclid_dist_if_necessary')” is different from the parameters in the code: paddle.cdist(x, y, p=2.0, compute_mode='use_mm_for_euclid_dist_if_necessary')”

Got it.

Copy link
Contributor

@sunzhongkai588 sunzhongkai588 left a comment

Choose a reason for hiding this comment

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

LGTM for docs,中文文档链接之后请附上

Copy link
Contributor

@jeff41404 jeff41404 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
Copy link
Contributor

luotao1 commented Jun 2, 2023

image@XieYunshen 看下单测时间,超时的理由在 https://github.com//pull/53836#issuecomment-1566777967

Copy link
Contributor

@XieYunshen XieYunshen 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 5b4d786 into PaddlePaddle:develop Jun 2, 2023
25 checks passed
@GreatV GreatV deleted the add_cdist_op branch July 2, 2023 15:06
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

8 participants