add reduce=True argument to MultiLabelMarginLoss #4924

li-roy · 2018-01-30T00:54:08Z

As per #264. When reduce is False, MultiLabelMarginLoss outputs a loss per sample in minibatch. When reduce is True (default), the current behavior is kept.

Test Plan
test/run_test.sh
Added unit test. For the reduce=False case. Added unit tests for 1d tensors. Added a reference function.

pytorchbot · 2018-01-30T00:54:09Z

@li-roy, thanks for your PR! We identified @zdevito to be a potential reviewer.

li-roy · 2018-01-30T01:24:54Z

@pytorchbot retest this please

zou3519 · 2018-01-30T02:06:33Z

@pytorchbot retest this please

zou3519

Haven't reviewed the C code or kernels yet, will get to that soon. I left a few comments on the python side to start out

aten/src/THCUNN/MultiLabelMarginCriterion.cu


 template <typename Dtype, typename Acctype>
 __global__ void cunn_MultiLabelMarginCriterion_updateGradInput_kernel(Dtype *gradInput,
+                                                                      Dtype *gradOutput,


torch/nn/modules/loss.py

+            size_average. Default: True
+
+    Shape:
+        - Input: :math:`(N)` or :math:`(N, *)` where `*` means, any number of additional


torch/nn/modules/loss.py

+        - Input: :math:`(N)` or :math:`(N, *)` where `*` means, any number of additional
+          dimensions
+        - Target: :math:`(N)` or :math:`(N, *)`, same shape as the input
+        - Output: scalar. If `reduce` is False, then `(N)` or `(N, *)`, same shape as


torch/nn/modules/loss.py


    `y` and `x` must have the same size.

    The criterion only considers the first non-negative `y[j]` targets.


test/test_nn.py

+
+
+def multilabelmarginloss_no_reduce_test():
+    t = Variable(torch.rand(5, 10).mul(10).floor().long())


test/common_nn.py

        check_no_size_average=True,
    ),
+    dict(
+        module_name='MultiLabelMarginLoss',


test/common_nn.py

+    if input.dim() == 1:
+        n = 1
+        dim = input.size()[0]
+        output = torch.Tensor(n).zero_()


test/common_nn.py

    return output


+def _multilabelmarginloss_reference(input, target, is_target):


test/common_nn.py

+
+    if input.dim() == 1:
+        n = 1
+        dim = input.size()[0]


test/common_nn.py

+def _multilabelmarginloss_reference(input, target, is_target):
+    sum = 0
+    for i in range(0, target.size()[0]):
+        target_index = target[i]


zou3519

The THNN code looks good for the most part! Just a few nits here and there. Haven't looked at the cuda kernel yet.

aten/src/THNN/generic/MultiLabelMarginCriterion.c

    isTarget_data += dim;
    gradInput_data += dim;
  }
+  gradInput_data -= nframe*dim;


aten/src/THNN/generic/MultiLabelMarginCriterion.c

    }
+
+    sum /= dim;
+    THTensor_(set1d)(output, t, sum);


aten/src/THNN/generic/MultiLabelMarginCriterion.c

+  else
+  {
+    THNN_CHECK_DIM_SIZE(gradOutput, 1, 0, nframe);
+    gradOutput = THTensor_(newContiguous)(gradOutput);


aten/src/THNN/generic/MultiLabelMarginCriterion.c

  THTensor_(free)(input);
  THIndexTensor_(free)(target);
  THTensor_(free)(isTarget);
+  THTensor_(free)(gradOutput);


torch/nn/modules/loss.py

+        reduce (bool, optional): By default, the losses are averaged or summed over
+            observations for each minibatch depending on size_average. When reduce
+            is False, returns a loss per batch element instead and ignores
+            size_average. Default: True


aten/src/THCUNN/MultiLabelMarginCriterion.cu

    if (threadIdx.x == 0) {
      gradInput_k[target_idx] += ScalarConvert<Acctype, Dtype>::to(totalSum);
    }
    __syncthreads();


torch/nn/modules/loss.py

+        reduce (bool, optional): By default, the losses are averaged or summed over
+            observations for each minibatch depending on size_average. When reduce
+            is False, returns a loss per batch element instead and ignores
+            size_average. Default: True


torch/nn/modules/loss.py

+            size_average is set to ``False``, the losses are instead summed for
+            each minibatch. Default: ``True``
+        reduce (bool, optional): By default, the losses are averaged or summed over
+            observations for each minibatch depending on size_average. When reduce


aten/src/THNN/generic/MultiLabelMarginCriterion.c

  THIndexTensor_(free)(target);
  THTensor_(free)(isTarget);
-  THTensor_(free)(gradOutput);
+  THTensor_(free)(gradInput);


zou3519

All comments addressed, LGTM unless anyone has something else to add. Thanks @li-roy!

ssnl · 2018-02-02T21:51:56Z

Thanks @li-roy !

zou3519 · 2018-02-02T21:55:13Z

I'm not sure why the CI isn't running, but... let's give this one final test

zou3519 · 2018-02-02T21:55:22Z

@pytorchbot retest this please

add reduce=True argument to MultiLabelMarginLoss

b93ada4

Fix lint

212c331

li-roy closed this Jan 30, 2018

li-roy reopened this Jan 30, 2018

zou3519 reviewed Jan 31, 2018

View reviewed changes

Addressed comments

25ff953

ssnl reviewed Feb 1, 2018

View reviewed changes

zou3519 reviewed Feb 2, 2018

View reviewed changes

Remove unneeded syncthreads calls

11e85a0

zou3519 approved these changes Feb 2, 2018

View reviewed changes

soumith merged commit 28f056f into pytorch:master Feb 5, 2018

soumith added 0.3.1 and removed 0.3.1 labels Feb 5, 2018

li-roy deleted the multilabelmarginloss_reduce branch February 22, 2018 00:17


		`y` and `x` must have the same size.

		The criterion only considers the first non-negative `y[j]` targets.



		def multilabelmarginloss_no_reduce_test():
		t = Variable(torch.rand(5, 10).mul(10).floor().long())

		return output


		def _multilabelmarginloss_reference(input, target, is_target):

add reduce=True argument to MultiLabelMarginLoss #4924

add reduce=True argument to MultiLabelMarginLoss #4924

Uh oh!

Conversation

li-roy commented Jan 30, 2018

Uh oh!

pytorchbot commented Jan 30, 2018

Uh oh!

li-roy commented Jan 30, 2018

Uh oh!

zou3519 commented Jan 30, 2018

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

ssnl commented Feb 2, 2018

Uh oh!

zou3519 commented Feb 2, 2018