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.

Sign up for GitHub

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

Jump to bottom

【Hachathon No.30】 #40545

Merged
merged 41 commits into from Jun 13, 2022
Merged

【Hachathon No.30】 #40545

merged 41 commits into from Jun 13, 2022

Conversation

yangguohao
Copy link
Contributor

@yangguohao yangguohao commented Mar 14, 2022 •
edited

PR types

New features

PR changes

APIs

Describe

解决了issues：#40303
添加了paddle.nn.TripletMarginWithDistanceLoss以及paddle.nn.functional.triplet_margin_with_distance_loss的函数
设计文档：PaddlePaddle/community#34
中文文档PR链接:PaddlePaddle/docs#4644

@paddle-bot-old
Copy link

paddle-bot-old bot commented Mar 14, 2022 •
edited

✅ This PR's description meets the template requirements!
Please wait for other CI results.

@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@CLAassistant
Copy link

CLAassistant commented Mar 14, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@yangguohao yangguohao mentioned this pull request Mar 14, 2022
Closed
@dingjiaweiww
Copy link
Contributor

你的 PR 提交成功，感谢你对于开源项目的贡献

@dingjiaweiww
Copy link
Contributor

PR 格式检查通过，你的PR 将接受Paddle 专家以及开源社区的review，请及时关注PR 动态

@shiyutang
Copy link
Contributor

根据流程，方案设计通过之后才能开始代码review，请仔细查看对应方案设计文档PR的修改意见，修改之后对应进行代码部分修改，当文档合入之后方可进行代码 PR 的review。

@shiyutang
Copy link
Contributor

根据 CI 报错信息来看，你需要将paddle dev merge到当前 yangguohao:triplet_margin_distance_loss 分支来通过CI
image

shiyutang
shiyutang suggested changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@shiyutang shiyutang left a comment •
edited

Choose a reason for hiding this comment

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

需要重新拉取develop的代码，通过所有CI（除PR-CI-Static-Check）之后才能进行进一步review

@dingjiaweiww
Copy link
Contributor

请先通过CI噢~

1 similar comment
@dingjiaweiww
Copy link
Contributor

请先通过CI噢~

@shiyutang
Copy link
Contributor

shiyutang commented Mar 31, 2022 •
edited

  1. 需要增加设计文档链接，如图所示：
    06b8a2904adf5ec3c229031619c25f57

  2. 需要替换 in_dygraph_mode 如下图所示：
    4ac43d3d67cdea50a08659a006f90a19

@yangguohao yangguohao force-pushed the triplet_margin_distance_loss branch from d534091 to a15eae3 Compare April 5, 2022 12:11
Ligoml
Ligoml previously approved these changes May 20, 2022
View reviewed changes
Copy link
Contributor

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

jeff41404
jeff41404 reviewed May 20, 2022
View reviewed changes
python/paddle/nn/layer/loss.py Outdated
# Tensor([0.19165580])

"""
def __init__(self, distance_function=None, margin=1.0, swap=False, reduction: str = 'mean', name=None):
Copy link
Contributor

@jeff41404 jeff41404 May 20, 2022 •
edited

Choose a reason for hiding this comment

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

the order of parameters here must same with RFC, here suggest to modify RFC distance_function before margin, and class name TripletMarginDistanceLoss in RFC should be consistance with TripletMarginWithDistanceLoss here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已重提PR修改RFC

shiyutang
shiyutang previously approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@shiyutang shiyutang left a comment •
edited

Choose a reason for hiding this comment

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

LGTM，确认和RFC一致。

Ligoml
Ligoml previously approved these changes May 31, 2022
View reviewed changes
jeff41404
jeff41404 previously approved these changes Jun 1, 2022
View reviewed changes
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

@yangguohao yangguohao dismissed stale reviews from jeff41404, Ligoml, and shiyutang via 2a4ed81 June 1, 2022 03:02
XiaoguangHu01
XiaoguangHu01 previously approved these changes Jun 1, 2022
View reviewed changes
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

Ligoml
Ligoml previously approved these changes Jun 2, 2022
View reviewed changes
Ligoml
Ligoml previously approved these changes Jun 8, 2022
View reviewed changes
XiaoguangHu01
XiaoguangHu01 approved these changes Jun 10, 2022
View reviewed changes
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 5d48528 into PaddlePaddle:develop Jun 13, 2022
@yangguohao yangguohao deleted the triplet_margin_distance_loss branch September 18, 2023 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ligoml Ligoml Ligoml approved these changes

@XiaoguangHu01 XiaoguangHu01 XiaoguangHu01 approved these changes

@jeff41404 jeff41404 jeff41404 left review comments

@DDDivano DDDivano DDDivano left review comments

@shiyutang shiyutang shiyutang left review comments

Assignees

@DDDivano DDDivano

@shiyutang shiyutang

Labels
contributor External developers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@yangguohao @CLAassistant @dingjiaweiww @shiyutang @jeff41404 @DDDivano @Ligoml @XiaoguangHu01