-
Notifications
You must be signed in to change notification settings - Fork 270
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】 #52
Conversation
PR格式检查通过,你的PR将接受Paddle专家以及开源社区的review,请及时关注PR动态。 |
CosineEmbeddingLoss的API设计为`torch.nn.CosineEmbeddingLoss(margin=0, reduction='mean')`,cosine_embedding_loss的API设计为`torch.nn.functional.cosine_embedding_loss(x1, x2, target)` | ||
|
||
整体设计与paddle保持一致,其中: |
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.
这里存在两个问题:
- torch / paddle 写反了;
- 对比竞品,这里设计的参数少了一些,需要明确写一下这么设计的理由。
1. 使用`paddle.zero`初始化结果列表 | ||
2. 使用`paddle.matmul`实现向量点乘 | ||
3. 使用`paddle.norm`实现向量二次范数相乘 | ||
4. 使用paddle API`sum` 和`mean` 实现reduction计算 |
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.
从竞品实现方式看,对正负样本的处理是有一些区别的;在该设计中是否有呢?如有,建议在此处明确写一下。
|
||
- 动态图,静态图,与numpy的结果保持一致; | ||
- 输入含`NaN`结果的正确性; | ||
- 错误检查:`input`和 `target`维度不为不合规时能抛出输入维度错误; |
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.
这里两个问题:
维度不为不合规
,语句有些不通顺- 测试中需要包含涉及的参数,在此处需要补充一下margin/reduction等参数的测试
1. 首先判定输入维度是否正确 | ||
2. 计算向量点乘`prod_sum`和向量二阶范式乘积`denom` | ||
3. 求出余弦相似度`cos = prod_sum / denom` | ||
4. 计算出正负样本损失值总和`output` |
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.
竞品代码中对正负样本的处理过程,也可以在这里补充一下
针对上述建议已进行修改,请查阅。 |
你的PR有最新反馈,请及时修改。 |
|
||
CosineEmbeddingLoss的API设计为`paddle.nn.CosineEmbeddingLoss(margin=0, reduction='mean')`,cosine_embedding_loss的API设计为`paddle.nn.functional.cosine_embedding_loss(x1, x2, target, margin=0, reduction='mean')` | ||
|
||
整体设计与paddle保持一致,其中: |
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.
这个地方还需要改一下,与Pytorch
一致
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.
这里与pytorch的差异在于size_average
和reduce
两个参数,在设计文档有提到这两个参数在pytorch中已经弃用了,请问与pytorch保持一致的话不实现具体功能可以吗
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.
这里可能是我没表达清楚意思:
- 关于
size_average
和reduce
的差异,之前的设计已经写得挺清楚了,没有问题。这里主要是指整体设计与paddle保持一致
这句话有语病的小问题,当前是为paddle新增的API,为什么会与paddle保持一致?这里直接删掉这句话是否更清晰一些 - 参数的设置上,根据此前的调研结论,不必再新增
size_average
和reduce
,仍然保持上一版的设计即可。
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.
了解,已修改
- 错误检查:`input`和 `target`维度不合规时能抛出输入维度错误; | ||
- 错误检查:`margin`设置超出[-1, 1]范围时抛出参数设置错误; | ||
- 错误检查:`reduction`设置除`sum` 和`mean`以外时抛出参数设置错误; | ||
|
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.
这里还请参考下API测试内容及单元测试要求的几点内容补充一下测试用例的设计:例如gpu/cpu;支持的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.
LGTM
你的PR已合入community库,请进行后续代码开发,并将代码提交至Paddle仓库。 |
为 Paddle 新增 paddle.nn.CosineEmbeddingLoss 和 paddle.nn.functional.cosine_embedding_loss API