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

【Paddle Hackathon No.11】 #45595

Merged
merged 14 commits into from
Oct 13, 2022
Merged

Conversation

yangguohao
Copy link
Contributor

@yangguohao yangguohao commented Aug 30, 2022

PR types

New features

PR changes

APIs

Describe

解决 issues #44073 (comment)
添加了paddle.nn.MultiMarginLoss以及paddle.nn.functional.multi_margin_loss的函数
设计文档 PaddlePaddle/community#171
中文文档 PaddlePaddle/docs#5262

@paddle-bot-old paddle-bot-old bot added the contributor External developers label Sep 1, 2022
prog = paddle.static.Program()
startup_prog = paddle.static.Program()
with paddle.static.program_guard(prog, startup_prog):
input = paddle.static.data(name='input',

Choose a reason for hiding this comment

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

For test cases in static mode, you'd better add some cases that paddle.static.data creates data layer that has dynamic shape to ensure that it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add the new test cases of checking data shape in static mode. but I'm not quite sure about it.

Choose a reason for hiding this comment

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

I mean, you can create an data layer with shape [-1, -1] and feed it with an array with shape [3, 4] or [5, 6].
Or create a data layer with shape [-1, 3,4] and feed it with an array with shape [7, 3, 4]

I the rank and all the static size matches, it is compatible.

Copy link

@iclementine iclementine left a comment

Choose a reason for hiding this comment

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

  1. Weight should be for each class, instead of each example. The main logic is different than the design in RFC;
  2. use paddle.shape(tensor) to get actual shape so it can work with dynamic shape in static mode;
  3. add test cases for dynamic shape in static mode;
  4. The documentation should be modified.

Copy link

@iclementine iclementine left a comment

Choose a reason for hiding this comment

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

The computation does not agree with the formula in the documentation.

Copy link

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

@iclementine iclementine left a comment

Choose a reason for hiding this comment

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

The CI system says

2022-09-26 16:20:48 0. It is recommended to use 'np.testing.assert_allclose' and 'np.testing.array_equal' instead of 'self.assertTrue(np.allclose(...))' and 'self.assertTrue(np.array_equal(...))'.

Also, I suggest a simpler implementation to pick N values from weight of shape (C,). See the comment above.

@yangguohao
Copy link
Contributor Author

The CI system says

2022-09-26 16:20:48 0. It is recommended to use 'np.testing.assert_allclose' and 'np.testing.array_equal' instead of 'self.assertTrue(np.allclose(...))' and 'self.assertTrue(np.array_equal(...))'.

Also, I suggest a simpler implementation to pick N values from weight of shape (C,). See the comment above.

Done, change 'self.asserrTrue' to 'np.testing.assert_allclose'

iclementine
iclementine previously approved these changes Sep 29, 2022
Copy link

@iclementine iclementine left a comment

Choose a reason for hiding this comment

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

LGTM

python/paddle/nn/layer/loss.py Outdated Show resolved Hide resolved
python/paddle/nn/layer/loss.py Show resolved Hide resolved
python/paddle/nn/layer/loss.py Show resolved Hide resolved
reduction='mean',
name=None):
r"""
Measures a multi-class classification hinge loss (margin-based loss) between input :math:`input` (a 2D mini-batch `Tensor`, in shape (N, C),
Copy link
Contributor

Choose a reason for hiding this comment

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

参考paddle.nn.MultiMarginLoss做修改吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

iclementine
iclementine previously approved these changes Sep 30, 2022

label: 1-D Tensor, the shape is [N,].

output: scalar. If :attr:`reduction` is ``'none'``, then same shape as the input.
Copy link
Contributor

Choose a reason for hiding this comment

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

image

好像不是一个意思?input不是2-D tensor吗?中文文档中的返回是一个1-D tensor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

应该是和 label 同一维度,能否合入后,我再发起 pr 修改

Copy link
Contributor

Choose a reason for hiding this comment

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

文档修改可以在commit后加上 ; test=document_fix 跳过代码检查的CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

margin: float = 1.0,
weight=None,
reduction="mean",
name=None):
Copy link
Contributor

@jeff41404 jeff41404 Oct 10, 2022

Choose a reason for hiding this comment

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

in order to have rfc same with code, need to modify paddle.nn.MultiMarginLoss( p:int = 1, margin:float=1.0, weight (Tensor,可选), reduction(str,可选), name=None) -> None: in rfc 命名与参数设计

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

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

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

@Ligoml
Copy link
Contributor

Ligoml commented Oct 12, 2022

image

@yangguohao 啊哦,示例代码挂了,重新修改一下吧,可以采用 ; test=document_fix 的方式跳过代码检查的CI

@SigureMo
Copy link
Member

SigureMo commented Oct 12, 2022

@yangguohao 啊哦,示例代码挂了,重新修改一下吧,可以采用 ; test=document_fix 的方式跳过代码检查的CI

该问题应由 #46712 导致,最近的只要修改了文档的 PR 均存在此问题,#46944 merge 后应可解决本问题

@yangguohao 需要 #46944 merge 后,重新 merge 下 develop

@Ligoml Ligoml merged commit 8474392 into PaddlePaddle:develop Oct 13, 2022
@yangguohao yangguohao deleted the multi_margin_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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants