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

Support combined_margin_loss op in flow.nn.modules #5830

Merged

oneflow-ci-bot merged 9 commits into master from move_combined_margin_loss

Aug 16, 2021

Contributor

tingkuanpei commented Aug 11, 2021 •

edited

Loading

tingkuanpei requested a review from doombeaker

August 11, 2021 03:54

CLAassistant commented Aug 11, 2021 •

edited

Loading

All committers have signed the CLA.

tingkuanpei force-pushed the move_combined_margin_loss branch 3 times, most recently from b63597e to 31cac18 Compare

August 11, 2021 12:12

doombeaker reviewed

View reviewed changes

python/oneflow/compatible/single_client/test/ops/test_combined_margin_loss.py Outdated

    
            @@ -64,7 +64,7 @@ def test_combined_margin_loss(
          
              ):

                  assert device_type in ["gpu", "cpu"]

                  flow.clear_default_session()

                  flow.config.gpu_device_num(4)

                  flow.config.gpu_device_num(1)

Contributor

doombeaker Aug 11, 2021 •

edited

Loading

single_client 目录是为了兼容0.4.0及其之前的代码不得不保留的。已经不再更新了。
也就是这个目录下不需要做更改。测试也不需要写以前的那种lazy模式的，基于 @global_function 的测试了。

完成 python/oneflow/test/modules/test_combined_margin_loss.py 下的测试文件就可以了

Contributor Author

tingkuanpei Aug 11, 2021

明白，代码还没有开发完，有一些调试的代码，会在开发完后删掉。

tingkuanpei force-pushed the move_combined_margin_loss branch 5 times, most recently from c65e116 to b2c8791 Compare

August 13, 2021 06:35


          Support combined_margin_loss op in flow.nn.modules

c24b428

tingkuanpei force-pushed the move_combined_margin_loss branch from b2c8791 to c24b428 Compare

August 13, 2021 06:36

doombeaker reviewed

View reviewed changes

oneflow/core/autograd/gradient_funcs/combined_margin_loss.cpp Show resolved Hide resolved

python/oneflow/nn/modules/combined_margin_loss.py Outdated

+                      >>> np_label = np.random.randint(0, 6, size=(10)).astype(np.int32)
+                      >>> x = flow.Tensor(x, dtype=flow.float32)
+                      >>> label = flow.Tensor(label, dtype=flow.int32)
+                      >>> out = flow.combined_margin_loss(x, label, 0.3, 0.5, 0.4)

Contributor

doombeaker Aug 13, 2021 •

edited

Loading

这个 docstring 需要切换到
cd ~/oneflow/docs
目录下，安装那个目录下的 requirements.txt 依赖。然后运行

make html SPHINXOPTS="-W --keep-going"

得到 docs/build/html 目录，在里面查看编译生成的文档。

如果编译docs过程有报错（包含警告），也需要检查修正下（CI也要求文档没格式错误才会允许合并）

Contributor

doombeaker Aug 13, 2021

要检测 doctest，只需要运行这个文件即可：

python combined_margin_loss.py

python/oneflow/nn/modules/combined_margin_loss.py Outdated

+                      >>> import numpy as np
+                      >>> np_x = np.random.uniform(low=-1, high=1, size=(10, 6)).astype(np.float32)
+                      >>> np_label = np.random.randint(0, 6, size=(10)).astype(np.int32)
+                      >>> x = flow.Tensor(x, dtype=flow.float32)

Contributor

doombeaker Aug 13, 2021

Suggested change

      
                    >>> x = flow.Tensor(x, dtype=flow.float32)
          
                    >>> x = flow.Tensor(np_x, dtype=flow.float32)

python/oneflow/nn/modules/combined_margin_loss.py Outdated

+                      >>> np_x = np.random.uniform(low=-1, high=1, size=(10, 6)).astype(np.float32)
+                      >>> np_label = np.random.randint(0, 6, size=(10)).astype(np.int32)
+                      >>> x = flow.Tensor(x, dtype=flow.float32)
+                      >>> label = flow.Tensor(label, dtype=flow.int32)

Contributor

doombeaker Aug 13, 2021

Suggested change

      
                    >>> label = flow.Tensor(label, dtype=flow.int32)   
          
                    >>> label = flow.Tensor(np_label, dtype=flow.int32)

python/oneflow/nn/modules/combined_margin_loss.py Outdated

+                      >>> x = flow.Tensor(x, dtype=flow.float32)
+                      >>> label = flow.Tensor(label, dtype=flow.int32)
+                      >>> out = flow.combined_margin_loss(x, label, 0.3, 0.5, 0.4)

Contributor

doombeaker Aug 13, 2021

这里还要加一个 >>> out 并且写上预期的输出，否则doctest没有预期输出，就失去了 test 的意义。

python/oneflow/nn/modules/combined_margin_loss.py Outdated

		return y


		def combined_margin_loss_op(x, label, m1: float = 1, m2: float = 0, m3: float = 0):

Contributor

doombeaker Aug 13, 2021

我感觉，可以按照 pytorch Module 的编程习惯，不用导出这种 functional 类型的 op 接口了。
直接导出 CombinedMarginLoss 类就可以了（参考 loss.py 里的各类）。

如果要保留这种 functional 式的接口，那么，就不需要实现 CombinedMarginLoss 类先实例化再调用，而直接调用 flow.F.combined_margin_loss。

我比较倾向于前一种方式，看你的意思呢？

Contributor Author

tingkuanpei Aug 13, 2021

我按照第一种方式改，然后把class CombinedMarginLoss(Module) 也移动到loss.py里？

python/oneflow/test/modules/test_combined_margin_loss.py Show resolved Hide resolved

tingkuanpei and others added 2 commits

August 13, 2021 09:51


          Follow review comment to modify

3d8c029


          Merge branch 'master' into move_combined_margin_loss

b4e5e1c

doombeaker reviewed

View reviewed changes

python/oneflow/nn/modules/loss.py Outdated

@@ @@ -1162,6 +1162,48 @@ def forward(self, input, target) -> Tensor: @@
                           return flow.mean(loss)
+              class CombinedMarginLoss(Module):
+                  """The operation implement "loss_name == 'margin_softmax'" in insightface.

Contributor

doombeaker Aug 13, 2021

这个 loss 的资料其实还不是属于“常识类”的吧，要不给个文献的出处？pytorch 和我们的源码里又多处这种情况。像

oneflow/python/oneflow/nn/optimizer/rmsprop.py

Line 29 in c071635

http://www.cs.toronto.edu/~tijmen/csc321/slides/lecture_slides_lec6.pdf .

这些算子这样。

python/oneflow/nn/modules/loss.py Outdated

@@ @@ -1162,6 +1162,48 @@ def forward(self, input, target) -> Tensor: @@
                           return flow.mean(loss)
+              class CombinedMarginLoss(Module):
+                  """The operation implement "loss_name == 'margin_softmax'" in insightface.

Contributor

doombeaker Aug 13, 2021

Suggested change

      
                """The operation implement "loss_name == 'margin_softmax'" in insightface.
          
                """The operation implements "margin_softmax" in InsightFace.

然后给个文献链接，这样是不是更方便读者了解？

oneflow/user/kernels/combined_margin_loss_kernel.cu Outdated Show resolved Hide resolved

python/oneflow/nn/modules/loss.py Outdated

+                  def forward(self, x, label):
+                      depth = x.shape[1]
+                      (y, theta) = flow.F.combined_margin_loss(x, label,

Contributor

doombeaker Aug 13, 2021

Suggested change

      
                    (y, theta) = flow.F.combined_margin_loss(x, label,
          
                    (y, _) = flow.F.combined_margin_loss(x, label,

有些静态分析的工具，对不使用的变量会报警告，建议按照习惯，不用的变量名字采用 _

python/oneflow/nn/modules/loss.py Outdated

@@ @@ -1162,6 +1162,48 @@ def forward(self, input, target) -> Tensor: @@
                           return flow.mean(loss)
+              class CombinedMarginLoss(Module):
+                  """The operation implement "loss_name == 'margin_softmax'" in insightface.
+                  insightface's margin_softmax loss implement by several operators, we combined them for speed up.

Contributor

doombeaker Aug 13, 2021

Suggested change

      
                insightface's margin_softmax loss implement by several operators, we combined them for speed up.
          
                The implementation of margin_softmax in InsightFace is composed of multiple operators. We fuse them for speed up.

我看算子融合会用 “fuse”，是不是比 combine 要准确一点（这个可以再和郭冉确认下）？
另外 InsightFace 的大小写要注意统一更改下。

oneflow/user/kernels/combined_margin_loss_kernel.cpp Outdated Show resolved Hide resolved

python/oneflow/test/modules/test_combined_margin_loss.py Outdated Show resolved Hide resolved

doombeaker and others added 3 commits

August 13, 2021 06:05


          Merge branch 'master' into move_combined_margin_loss

16c35ef


          Follow review comment to modify

f545832


          Merge branch 'master' into move_combined_margin_loss

9e5000d

doombeaker approved these changes

View reviewed changes

doombeaker requested a review from oneflow-ci-bot

August 16, 2021 04:16

doombeaker added enhancement op automerge and removed enhancement op labels


          auto format by CI

e3cf1de


          Merge branch 'master' into move_combined_margin_loss

789750b

oneflow-ci-bot requested review from oneflow-ci-bot and removed request for oneflow-ci-bot

August 16, 2021 05:48


          Merge branch 'master' into move_combined_margin_loss

b4c0d09

oneflow-ci-bot self-requested a review

August 16, 2021 06:28

Contributor

github-actions bot commented Aug 16, 2021

Speed stats:

GPU Name: GeForce GTX 1080 

PyTorch resnet50 time: 146.7ms (= 7332.9ms / 50, input_shape=[16, 3, 224, 224], backward is enabled)
OneFlow resnet50 time: 128.5ms (= 6427.2ms / 50, input_shape=[16, 3, 224, 224], backward is enabled)
Relative speed: 1.14 (= 146.7ms / 128.5ms)

PyTorch resnet50 time: 84.4ms (= 4219.9ms / 50, input_shape=[8, 3, 224, 224], backward is enabled)
OneFlow resnet50 time: 74.5ms (= 3723.5ms / 50, input_shape=[8, 3, 224, 224], backward is enabled)
Relative speed: 1.13 (= 84.4ms / 74.5ms)

PyTorch resnet50 time: 57.4ms (= 2872.2ms / 50, input_shape=[4, 3, 224, 224], backward is enabled)
OneFlow resnet50 time: 47.3ms (= 2362.9ms / 50, input_shape=[4, 3, 224, 224], backward is enabled)
Relative speed: 1.22 (= 57.4ms / 47.3ms)

PyTorch resnet50 time: 50.6ms (= 2530.9ms / 50, input_shape=[2, 3, 224, 224], backward is enabled)
OneFlow resnet50 time: 39.4ms (= 1968.4ms / 50, input_shape=[2, 3, 224, 224], backward is enabled)
Relative speed: 1.29 (= 50.6ms / 39.4ms)

PyTorch resnet50 time: 43.0ms (= 2152.3ms / 50, input_shape=[1, 3, 224, 224], backward is enabled)
OneFlow resnet50 time: 36.7ms (= 1833.1ms / 50, input_shape=[1, 3, 224, 224], backward is enabled)
Relative speed: 1.17 (= 43.0ms / 36.7ms)

oneflow-ci-bot merged commit 04bb36c into master

oneflow-ci-bot deleted the move_combined_margin_loss branch

August 16, 2021 07:31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge enhancement op