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.17】为 Paddle 新增 paddle.nn.CosineEmbeddingLoss 和 paddle.nn.functional.cosine_embedding_loss API #41680

Merged
merged 36 commits into from Jun 8, 2022

Conversation

Patrick-Star125
Copy link
Contributor

@Patrick-Star125 Patrick-Star125 commented Apr 12, 2022

PR types

Others

PR changes

APIs

Describe

  1. 解决了issue 【PaddlePaddle Hackathon 2】17、为 Paddle 新增 paddle.nn.CosineEmbeddingLoss 和 paddle.nn.functional.cosine_embedding_loss API #40316
  2. 增加了两个损失函数API paddle.nn.CosineEmbeddingLoss 和 paddle.nn.functional.cosine_embedding_loss
  3. 设计文档在 【Hackathon No.17】 community#52
  4. 中文文档【PaddlePaddle Hackathon 】No.17 add docs for API CosineEmbeddingLoss docs#4737

@CLAassistant
Copy link

CLAassistant commented Apr 12, 2022

CLA assistant check
All committers have signed the CLA.

@paddle-bot-old paddle-bot-old bot added contributor External developers status: proposed labels Apr 12, 2022
@paddle-bot-old
Copy link

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

@dingjiaweiww
Copy link
Contributor

请先通过CI检测噢~

@Asthestarsfalll
Copy link
Contributor

scalar的问题貌似是之前的一个pr有问题,已经revert了,重新merge一下develop分支即可

@paddle-bot-old
Copy link

PR格式检查通过,你的PR将接受Paddle专家以及开源社区的review,请及时关注PR动态。
The format inspection passed. Your PR will be reviewed by experts of Paddle and developers from the open-source community. Stay tuned.

@dingjiaweiww dingjiaweiww assigned zoooo0820 and unassigned betterpig Apr 18, 2022
Copy link
Contributor

@zoooo0820 zoooo0820 left a comment

Choose a reason for hiding this comment

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

当前CI有几个问题仍然需要关注:

  • Coverage:单测提示超时。请在保证覆盖率的前提下,适当减少功能重复或不必要的单测用例
  • Static: 代码风格报错,请按提示修改,或者用pre-commit刷下格式

from __future__ import print_function

import paddle
import paddle.fluid as fluid
Copy link
Contributor

Choose a reason for hiding this comment

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

因为fluid目前是非公开API并在逐步清理退场中。这里请不要使用fluid API,避免增加后续清理工作。静态图下相关API请参考paddle.static,设备相关检查请换用paddle.device.is_compiled_with_cuda()paddle.CPUPlace等等

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

elif reduction == 'sum':
return paddle.sum(scores)

fluid.data_feeder.check_variable_and_dtype(
Copy link
Contributor

@zoooo0820 zoooo0820 Apr 18, 2022

Choose a reason for hiding this comment

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

同单测文件,这里也请不要显式使用fluid API。在文件开始引用了check_variable_and_dtype,这里直接使用这个就行,方便后续清理工作

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

shape=[label.shape[0]],
dtype='{}'.format(label.dtype).replace("paddle.", ""),
value=0)
check_zero = fluid.layers.fill_constant(
Copy link
Contributor

Choose a reason for hiding this comment

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

同上,这里也请换用static下的api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改


class TestFunctionCosineEmbeddingLoss(unittest.TestCase):
def setUp(self):
self.input1_np = np.random.random(size=(3, 3)).astype(np.float64)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 单测中需要覆盖rfc设计文档中写到的测试用例,包括所有支持的dtype等
  • size可以适当设计一些不一样的case以及稍大的case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

在我线下Windows paddle-gpu-0.0.0环境中,dtype为float32的计算result和numpy结果except如下图所示

QQ截图20220420100550

差距在1e-8数量级上,应当是可以通过单测,但是不知道为什么只在window-inference中无法通过单测

出错代码在我上次commit中可以查看

@TCChenlong TCChenlong requested a review from DDDivano May 30, 2022 02:40
zoooo0820
zoooo0820 previously approved these changes May 30, 2022
@@ -2229,3 +2229,105 @@ def hinge_embedding_loss(input, label, margin=1.0, reduction='mean', name=None):
return paddle.sum(loss, name=name)
elif reduction == 'none':
return loss


def cosine_embedding_loss(input1, input2, label, margin=0, reduction='mean'):
Copy link
Contributor

@jeff41404 jeff41404 May 30, 2022

Choose a reason for hiding this comment

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

The parameters of x1, x2, target in paddle.nn.functional.cosine_embedding_loss 's RFC must be same with parameters here.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the design requirements, our API requires the name parameter like other loss API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now changing parameters will lead to the modification of code and document. Can I just change x1, x2, target to input1, input2, label in RFC?


"""

def __init__(self, margin=0, reduction='mean'):
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the design requirements, our API requires the name parameter like other loss API.

@Patrick-Star125 Patrick-Star125 dismissed stale reviews from zoooo0820 and TCChenlong via 95217b8 May 31, 2022 06:24
@TCChenlong TCChenlong closed this Jun 1, 2022
@TCChenlong TCChenlong reopened this Jun 1, 2022
@Patrick-Star125
Copy link
Contributor Author

已修改rfc,并且添加name参数及相关代码

@TCChenlong
Copy link
Contributor

@Patrick-Star125 Code Style 检查还是错了,需要用 pre-commit ~

@Patrick-Star125
Copy link
Contributor Author

已修改

Copy link
Contributor

@TCChenlong TCChenlong 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

@zoooo0820 zoooo0820 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

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

@jeff41404 jeff41404 merged commit 8dab269 into PaddlePaddle:develop Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants