You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/05/17 14:54:25 UTC

[GitHub] [tvm] zhuzilin opened a new pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

zhuzilin opened a new pull request #8056:
URL: https://github.com/apache/tvm/pull/8056


   This PR adds the `nll_loss` op to relay and topi, so that we could translate `aten::nll_loss` in pytorch frontend. The `nll_loss` is the underlying function for cross entropy in pytorch and is very important in the way of supporting training in tvm.
   
   Thank you for your time on reviewing this PR :).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r634036794



##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param input The input tensor.
+ * \param target The target tensor.
+ * \param weight A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& input, const Tensor& target, const Tensor& weight,
+                       std::string reduction = "mean", int ignore_index = -100,

Review comment:
       Yes, it's the default value used in pytorch, which is mainly a random negative number in my opinion.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-858375964


   @altanh @tkonolige @tqchen Is there anything I can help with this pr... It has been stuck for a while....


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-868658600


   @vinx13 @altanh @tkonolige @tqchen Thank you for your help! I'll submit a pr for the gradient of nll_loss soon.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin edited a comment on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin edited a comment on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-843696836


   @tkonolige Sorry about the incomplete formula... I don't know what  I was thinking about yesterday :scream:
   
   > please add unit tests for the op
   
   I found that the ops are categorized into different levels. Could you tell me how to decide to level of an op? And I could add the test to the correct place.
   
   > are you planning on registering a gradient for this operator?
   
   @altanh Yes, I'm trying to  write one for this op. I will try to implement one like the cpu version of pytorch or onnxruntime). It may take some time since it's a little complex...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-853290697


   It looks like we have some CI problems currently


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] tkonolige commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r651950458



##########
File path: tests/python/relay/test_op_level10.py
##########
@@ -577,6 +577,49 @@ def _verify(input_shape, diagonal_shape, dtype, k=0, align="RIGHT_LEFT"):
     _verify((2, 3, 4), (2, 4, 3), "int32", (-1, 2), "RIGHT_RIGHT")
 
 
+@tvm.testing.uses_gpu
+def test_nll_loss():

Review comment:
       Switch this to use `parameterize_targets`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-842864665


   @tkonolige 
   Thank you for your review! I've added the docs. And because the lint check doesn't allow parameter name as `input`, I renamed the parameters to be the same form as `cross_entropy` and `cross_entropy_with_logits` (the old one was the same as `aten::nll_loss`).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin edited a comment on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin edited a comment on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-868179165


   > in this case we need to change tag to `kInjective` as the reduction op is not broadcast
   
   @vinx13 Changing tag to `kInjective` will fail the tag check in `traverse_after_reduce` in
   https://github.com/apache/tvm/blob/d0791d3db971a111826d96201bd1e4c9c0d531da/python/tvm/topi/x86/reduction.py#L94
   whereas the empty tag was triggering `traverse_before_reduce`....
   
   It's worth noticing that the tag is of:
   
   ```c++
     auto T = tvm::te::compute(
         targets->shape,
         ...
         name, tag);
   ```
   
   which is an element-wise operation on `targets`. And when `reduction="mean"` or `reduction="sum"`, `T` will be reduced, whereas when `reduction="none"`, the `T` will be returned as the result of the `nll_loss`.
   
   Therefore, as `nll_loss` is registered to use reduce schedule, when `reduction="mean"` or `"sum"`, the `T` will go through the checks in `traverse_before_reduce` while when `reduction="none"`, `T` will go through checks in `traverse_after_reduce`. Right now, the `kBroadcast` happens to be the only tag to satisfiy both checks and passed the CI . To really solve this problem, I'm afraid we may need to define different tag and different scheduling for different op attr...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-847289962


   > @altanh Thanks. I'll add the test soon. Could you also check the comments above relate to the optional weight, Diagnostics and the tag?
   
   replied 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] tkonolige commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r634645616



##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -2973,6 +2973,40 @@ def cross_entropy_with_logits(predictions, targets):
     return _make.cross_entropy_with_logits(predictions, targets)
 
 
+def nll_loss(predictions, targets, weights, reduction="mean", ignore_index=-100):
+    """Negative log likelihood loss.
+
+    output{n, i_1, i_2, ..., i_k} = predictions{n, t, i_1, i_2, i_k}

Review comment:
       How does weights and ignore_index factor in here?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-852651910


   @altanh Could you take a look at the tests? Thank you~ And I wonder if there is any update with the `tag` and `OpPatternKind`?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin edited a comment on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin edited a comment on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-843696836


   @tkonolige Sorry about the incomplete formula... I don't know what  I was thinking about yesterday :scream:
   
   > are you planning on registering a gradient for this operator?
   
   @altanh Yes, I'm trying to  write one for this op. I will try to implement one like the cpu version of pytorch (https://github.com/pytorch/pytorch/blob/2ddd8416350b701fe0e7325e4761f8d14a3f46e5/aten/src/ATen/native/LossNLL.cpp#L29 and https://github.com/pytorch/pytorch/blob/2ddd8416350b701fe0e7325e4761f8d14a3f46e5/aten/src/ATen/native/LossNLL2d.cpp#L253). It may take some time since it's a little complex...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] vinx13 commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-867848988


   > @vinx13 @altanh Thank you for your help!
   > 
   > > tag here is topi-level, sometimes we use it to identify a specific compute operation during schedule, otherwise we can leave it empty
   > 
   > If I change the value of `tag` to an empty string, it will fail the check in `schedule_reduce`, which is:
   > 
   > https://github.com/apache/tvm/blob/d0791d3db971a111826d96201bd1e4c9c0d531da/python/tvm/topi/x86/reduction.py#L84
   > 
   > I'm not sure if I need to adjust somewhere else...
   
   in this case we need to change tag to `kInjective` as the reduction op is not broadcast


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r638226321



##########
File path: src/relay/op/nn/nn.cc
##########
@@ -1091,6 +1092,65 @@ Accept logits.
 // Depth to space and space to depth
 TVM_REGISTER_NODE_TYPE(SubPixelAttrs);
 
+// relay.nn.nll_loss
+TVM_REGISTER_NODE_TYPE(NLLLossAttrs);
+
+bool NLLLossRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+                const TypeReporter& reporter) {
+  ICHECK_EQ(types.size(), 4) << "NLLLossRel expects 4 types, but " << types.size()
+                             << " were provided.";
+  const auto* predictions = types[0].as<TensorTypeNode>();
+  const auto* targets = types[1].as<TensorTypeNode>();
+  const auto* weights = types[2].as<TensorTypeNode>();
+  const NLLLossAttrs* param = attrs.as<NLLLossAttrs>();
+  if (predictions == nullptr || targets == nullptr || weights == nullptr) return false;
+  ICHECK(predictions->shape.size() - targets->shape.size() == 1)
+      << "NLLLossRel: predictions should be one dimension larger than targets, "
+      << "predictions shape = " << predictions->shape << ", "
+      << "targets shape = " << targets->shape;
+  ICHECK(weights->shape.size() == 1)
+      << "NLLLossRel: weights should be a one dimension Tensor with its length "
+      << "the number of classes, but Tensor of dimension " << weights->shape.size()
+      << " were provided.";
+  ICHECK(reporter->AssertEQ(predictions->shape[1], weights->shape[0]))
+      << "NLLLossRel: the second dimension of predictions should be the number of classes, "
+      << "which is the length of weights, "
+      << "predictions shape = " << predictions->shape << ", "
+      << "weights shape = " << weights->shape;
+  ICHECK(predictions->dtype == weights->dtype && predictions->dtype.is_float())
+      << "NLLLossRel: predictions and weights should be of the same floating type.";
+  ICHECK(targets->dtype.is_int()) << "NLLLossRel: targets should be of int type.";

Review comment:
       basically, if the error can happen due to user input (e.g. using wrong shapes), we should definitely use diagnostics. ICHECK should be reserved only for internal compiler checks that should basically never fail unless there's a bug somewhere. The diagnostic framework is fairly new so a lot of old code still uses ICHECK incorrectly, we just need to slowly go through and update them unfortunately




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-868179165


   > in this case we need to change tag to `kInjective` as the reduction op is not broadcast
   
   @vinx13 Changing tag to `kInjective` will fail the tag check in `traverse_after_reduce` in
   https://github.com/apache/tvm/blob/d0791d3db971a111826d96201bd1e4c9c0d531da/python/tvm/topi/x86/reduction.py#L94
   whereas the empty tag was triggering `traverse_before_reduce`....
   
   It's worth noticing that the tag is of:
   
   ```c++
     auto T = tvm::te::compute(
         targets->shape,
         ...
         name, tag);
   ```
   
   which is an element-wise operation on `targets`. And when `reduction="mean"` or `reduction="sum"`, `T` will be reduced, whereas when `reduction="none"`, the `T` will be returned as the result of the `nll_loss`.
   
   Therefore, as `nll_loss` is registered to use reduce schedule, when `reduction="mean"` or `"sum"`, the `T` will go through the checks in `traverse_before_reduce` while when `reduction="none"`, `T` will go through checks in `traverse_after_reduce`. Right now, the `kBroadcast` happens to be the only tag to satisfiy both checks and passed the CI . To really solve this problem, I'm afraid we may need to define different tag for different op attr...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] vinx13 commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r657327709



##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param predictions The prediction tensor.
+ * \param targets The target tensor.
+ * \param weights A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& predictions, const Tensor& targets, const Tensor& weights,
+                       std::string reduction = "mean", int ignore_index = -100,
+                       const std::string name = "nll_loss", const std::string tag = kBroadcast) {

Review comment:
       tag here is topi-level, sometimes we use it to identify a specific `compute` operation during schedule, otherwise we can leave it empty




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r638229466



##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -2973,6 +2973,40 @@ def cross_entropy_with_logits(predictions, targets):
     return _make.cross_entropy_with_logits(predictions, targets)
 
 
+def nll_loss(predictions, targets, weights, reduction="mean", ignore_index=-100):
+    """Negative log likelihood loss.
+
+    output{n, i_1, i_2, ..., i_k} = predictions{n, t, i_1, i_2, i_k}
+      where t = target{n, i_1, i_2, ..., i_k}
+
+    result = reduction(output)
+
+    Parameters
+    ----------
+    predictions : tvm.relay.Expr
+      The predictions.
+
+    targets : tvm.relay.Expr
+      The target value of each prediction.
+
+    weights : tvm.relay.Expr

Review comment:
       Hmm not sure, that's a good point- let's just keep the weights for now. As for not needing a gradient, currently there is no other way than just putting some dummy value. It might make sense for us to introduce a `stop_gradient` dummy op which cuts the gradient computation from going further at undifferentiable arguments. Thanks!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-861118701


   > One last change and then I think this will be good to go.
   
   @tkonolige Could you point out the change that I need to make? Thank you~


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin edited a comment on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin edited a comment on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-843696836


   @tkonolige Sorry about the incomplete formula... I don't know what  I was thinking about yesterday :scream:
   
   > please add unit tests for the op
   
   I found that the ops are categorized into different levels. Could you tell me how to decide to level of an op? And I could add the test to the correct place.
   
   > are you planning on registering a gradient for this operator?
   
   @altanh Yes, I'm trying to  write one for this op. I will try to implement one like the cpu version of pytorch or onnxruntime).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r634912195



##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -2973,6 +2973,40 @@ def cross_entropy_with_logits(predictions, targets):
     return _make.cross_entropy_with_logits(predictions, targets)
 
 
+def nll_loss(predictions, targets, weights, reduction="mean", ignore_index=-100):
+    """Negative log likelihood loss.
+
+    output{n, i_1, i_2, ..., i_k} = predictions{n, t, i_1, i_2, i_k}
+      where t = target{n, i_1, i_2, ..., i_k}
+
+    result = reduction(output)
+
+    Parameters
+    ----------
+    predictions : tvm.relay.Expr
+      The predictions.
+
+    targets : tvm.relay.Expr
+      The target value of each prediction.
+
+    weights : tvm.relay.Expr

Review comment:
       @altanh We can make weights an optional parameter. I wonder if there are any example of a relay op with an optional tensor parameter that I can learn from. And also, how should we deal with gradient of an optional parameter? BTW, is there any better way we can mark a parameter as "no need for gradient" instead of returning an `one_like` grad?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-867050989


   > @tkonolige I've updated the test. But there seems to be some error with the cuda target. Could you give me some help? Thank you! Part of the error message in the CI is listed below:
   
   I think this error is due to using the default schedule (from `te.create_schedule`) which does not bind the iteration axes to the GPU threads. This is a bit outside of area of understanding but the solution will be to use the respective cuda/gpu schedules for each compute in the kernel (e.g. `schedule_reduce`, `schedule_injective`, etc.). Maybe @vinx13 can give some more detailed help?
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r633907154



##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param input The input tensor.
+ * \param target The target tensor.
+ * \param weight A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& input, const Tensor& target, const Tensor& weight,
+                       std::string reduction = "mean", int ignore_index = -100,

Review comment:
       I believe this is the default used in PyTorch, there's no inherent significance to it (although certain data loading/preprocessing scripts seem to try and match this value - e.g. I've see this in certain BERT training data loaders)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-859247826


   @tkonolige Thank you for the reviews! I've modified the parts based on them. Could you take another look?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r634875216



##########
File path: src/relay/op/nn/nn.cc
##########
@@ -1091,6 +1092,65 @@ Accept logits.
 // Depth to space and space to depth
 TVM_REGISTER_NODE_TYPE(SubPixelAttrs);
 
+// relay.nn.nll_loss
+TVM_REGISTER_NODE_TYPE(NLLLossAttrs);
+
+bool NLLLossRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+                const TypeReporter& reporter) {
+  ICHECK_EQ(types.size(), 4) << "NLLLossRel expects 4 types, but " << types.size()
+                             << " were provided.";
+  const auto* predictions = types[0].as<TensorTypeNode>();
+  const auto* targets = types[1].as<TensorTypeNode>();
+  const auto* weights = types[2].as<TensorTypeNode>();
+  const NLLLossAttrs* param = attrs.as<NLLLossAttrs>();
+  if (predictions == nullptr || targets == nullptr || weights == nullptr) return false;
+  ICHECK(predictions->shape.size() - targets->shape.size() == 1)
+      << "NLLLossRel: predictions should be one dimension larger than targets, "
+      << "predictions shape = " << predictions->shape << ", "
+      << "targets shape = " << targets->shape;
+  ICHECK(weights->shape.size() == 1)
+      << "NLLLossRel: weights should be a one dimension Tensor with its length "
+      << "the number of classes, but Tensor of dimension " << weights->shape.size()
+      << " were provided.";
+  ICHECK(reporter->AssertEQ(predictions->shape[1], weights->shape[0]))
+      << "NLLLossRel: the second dimension of predictions should be the number of classes, "
+      << "which is the length of weights, "
+      << "predictions shape = " << predictions->shape << ", "
+      << "weights shape = " << weights->shape;
+  ICHECK(predictions->dtype == weights->dtype && predictions->dtype.is_float())
+      << "NLLLossRel: predictions and weights should be of the same floating type.";
+  ICHECK(targets->dtype.is_int()) << "NLLLossRel: targets should be of int type.";

Review comment:
       Is there a plan on changing all `ICHECKS` to diagnostics? Or any rules on when to use `ICHECK` and when to use diagnostics?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r644227455



##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param predictions The prediction tensor.
+ * \param targets The target tensor.
+ * \param weights A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& predictions, const Tensor& targets, const Tensor& weights,
+                       std::string reduction = "mean", int ignore_index = -100,
+                       const std::string name = "nll_loss", const std::string tag = kBroadcast) {

Review comment:
       > @altanh Could you take a look at the tests? Thank you~ And I wonder if there is any update with the `tag` and `OpPatternKind`?
   
   I don't have much of an update for the tag, maybe you could try leaving it empty string? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] vinx13 commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-868652696


   This is merged, thanks @zhuzilin @altanh @tkonolige @tqchen 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r634783404



##########
File path: src/relay/op/nn/nn.cc
##########
@@ -1091,6 +1092,65 @@ Accept logits.
 // Depth to space and space to depth
 TVM_REGISTER_NODE_TYPE(SubPixelAttrs);
 
+// relay.nn.nll_loss
+TVM_REGISTER_NODE_TYPE(NLLLossAttrs);
+
+bool NLLLossRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+                const TypeReporter& reporter) {
+  ICHECK_EQ(types.size(), 4) << "NLLLossRel expects 4 types, but " << types.size()
+                             << " were provided.";
+  const auto* predictions = types[0].as<TensorTypeNode>();
+  const auto* targets = types[1].as<TensorTypeNode>();
+  const auto* weights = types[2].as<TensorTypeNode>();
+  const NLLLossAttrs* param = attrs.as<NLLLossAttrs>();
+  if (predictions == nullptr || targets == nullptr || weights == nullptr) return false;
+  ICHECK(predictions->shape.size() - targets->shape.size() == 1)
+      << "NLLLossRel: predictions should be one dimension larger than targets, "
+      << "predictions shape = " << predictions->shape << ", "
+      << "targets shape = " << targets->shape;
+  ICHECK(weights->shape.size() == 1)
+      << "NLLLossRel: weights should be a one dimension Tensor with its length "
+      << "the number of classes, but Tensor of dimension " << weights->shape.size()
+      << " were provided.";
+  ICHECK(reporter->AssertEQ(predictions->shape[1], weights->shape[0]))
+      << "NLLLossRel: the second dimension of predictions should be the number of classes, "
+      << "which is the length of weights, "
+      << "predictions shape = " << predictions->shape << ", "
+      << "weights shape = " << weights->shape;
+  ICHECK(predictions->dtype == weights->dtype && predictions->dtype.is_float())
+      << "NLLLossRel: predictions and weights should be of the same floating type.";
+  ICHECK(targets->dtype.is_int()) << "NLLLossRel: targets should be of int type.";

Review comment:
       can you replace these ICHECKs with Diagnostics?

##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param predictions The prediction tensor.
+ * \param targets The target tensor.
+ * \param weights A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation

Review comment:
       fix incorrect return docstring

##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -2973,6 +2973,40 @@ def cross_entropy_with_logits(predictions, targets):
     return _make.cross_entropy_with_logits(predictions, targets)
 
 
+def nll_loss(predictions, targets, weights, reduction="mean", ignore_index=-100):
+    """Negative log likelihood loss.
+
+    output{n, i_1, i_2, ..., i_k} = predictions{n, t, i_1, i_2, i_k}
+      where t = target{n, i_1, i_2, ..., i_k}
+
+    result = reduction(output)
+
+    Parameters
+    ----------
+    predictions : tvm.relay.Expr
+      The predictions.
+
+    targets : tvm.relay.Expr
+      The target value of each prediction.
+
+    weights : tvm.relay.Expr

Review comment:
       can we make weights optional, like PyTorch? weights=1 is a pretty common case I believe and we could add a fast path implementation that skips the scaling

##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param predictions The prediction tensor.
+ * \param targets The target tensor.
+ * \param weights A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& predictions, const Tensor& targets, const Tensor& weights,
+                       std::string reduction = "mean", int ignore_index = -100,
+                       const std::string name = "nll_loss", const std::string tag = kBroadcast) {

Review comment:
       should the tag be `kOpaque` to match the Relay pattern?

##########
File path: python/tvm/topi/nn/loss.py
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name,unused-argument
+"""Loss functions definitions."""
+from __future__ import absolute_import
+from . import cpp
+
+
+def nll_loss(predictions, targets, weights, reduction, ignore_index):
+    """Negative log likelihood loss on the input data.
+
+    output{n, i_1, i_2, ..., i_k} = predictions{n, t, i_1, i_2, i_k}

Review comment:
       missing minus sign in math? and weights

##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param predictions The prediction tensor.
+ * \param targets The target tensor.
+ * \param weights A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& predictions, const Tensor& targets, const Tensor& weights,
+                       std::string reduction = "mean", int ignore_index = -100,
+                       const std::string name = "nll_loss", const std::string tag = kBroadcast) {
+  auto T = tvm::te::compute(
+      targets->shape,
+      [&](const tvm::Array<tvm::tir::Var>& target_indices) {
+        auto c = targets(target_indices);
+        tvm::Array<tvm::PrimExpr> pred_indices;
+        for (size_t i = 0; i < target_indices.size(); i++) {
+          pred_indices.push_back(target_indices[i]);
+          if (i == 0) {
+            pred_indices.push_back(c);
+          }
+        }

Review comment:
       This is clearer to me
   ```suggestion
           pred_indices.push_back(target_indices[0]);  // batch index
           pred_indices.push_back(c); // class index
           for (size_t i = 1; i < target_indices.size(); i++) {
             pred_indices.push_back(target_indices[i]);  // indices for multidimensional loss
           }
   ```

##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -2973,6 +2973,40 @@ def cross_entropy_with_logits(predictions, targets):
     return _make.cross_entropy_with_logits(predictions, targets)
 
 
+def nll_loss(predictions, targets, weights, reduction="mean", ignore_index=-100):
+    """Negative log likelihood loss.
+
+    output{n, i_1, i_2, ..., i_k} = predictions{n, t, i_1, i_2, i_k}

Review comment:
       missing a minus sign in the math here?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-845452140


   > Could you tell me how to decide to level of an op? And I could add the test to the correct place.
   
   Yep, sorry that the documentation of support level is lacking! We do have a doc at (https://tvm.apache.org/docs/langref/relay_op.html#overview-of-operators) but it seems outdated and missing quite a bit, so this needs to be improved. For this op, I think we can go with level 10 which matches the existing `nn.cross_entropy` op. I think we might want to remove this operator in the future since NLLLoss seems to be more general and subsumes it in my opinion, but that will be a separate PR :)
   
   Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin edited a comment on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin edited a comment on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-843696836


   @tkonolige Sorry about the incomplete formula... I don't know what  I was thinking about yesterday :scream:
   
   > are you planning on registering a gradient for this operator?
   
   @altanh Yes, I'm trying to  write one for this op. I will try to implement one just like the cpu version of pytorch (https://github.com/pytorch/pytorch/blob/2ddd8416350b701fe0e7325e4761f8d14a3f46e5/aten/src/ATen/native/LossNLL.cpp#L29). It may take some time since it's a little complex...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r634868104



##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param predictions The prediction tensor.
+ * \param targets The target tensor.
+ * \param weights A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& predictions, const Tensor& targets, const Tensor& weights,
+                       std::string reduction = "mean", int ignore_index = -100,
+                       const std::string name = "nll_loss", const std::string tag = kBroadcast) {

Review comment:
       @altanh I am confused with the `tag` in topi (ones in `topi/tags.h`) and the `OpPatternKind` in `relay/op_attr_types.h`. It seems that they are not matched. Could you tell me the usage of them in tvm? Thank you~




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] tkonolige commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r633772853



##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param input The input tensor.
+ * \param target The target tensor.
+ * \param weight A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& input, const Tensor& target, const Tensor& weight,
+                       std::string reduction = "mean", int ignore_index = -100,

Review comment:
       What is the significance of the -100 for `ignore_index`?

##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -2973,6 +2973,34 @@ def cross_entropy_with_logits(predictions, targets):
     return _make.cross_entropy_with_logits(predictions, targets)
 
 
+def nll_loss(input, target, weight, reduction="mean", ignore_index=-100):
+    """Negative log likelihood loss.

Review comment:
       Can you document how `nll_loss` is computed (i.e. a mathematical expression).

##########
File path: src/relay/op/nn/nn.cc
##########
@@ -1091,6 +1092,60 @@ Accept logits.
 // Depth to space and space to depth
 TVM_REGISTER_NODE_TYPE(SubPixelAttrs);
 
+// relay.nn.nll_loss
+TVM_REGISTER_NODE_TYPE(NLLLossAttrs);
+
+bool NLLLossRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+                const TypeReporter& reporter) {
+  ICHECK_EQ(types.size(), 4);

Review comment:
       ```suggestion
     ICHECK_EQ(types.size(), 4) << "NLLLossRel expects 4 types, but " << types.size() << " were provided.";
   ```

##########
File path: src/relay/op/nn/nn.cc
##########
@@ -1091,6 +1092,60 @@ Accept logits.
 // Depth to space and space to depth
 TVM_REGISTER_NODE_TYPE(SubPixelAttrs);
 
+// relay.nn.nll_loss
+TVM_REGISTER_NODE_TYPE(NLLLossAttrs);
+
+bool NLLLossRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+                const TypeReporter& reporter) {
+  ICHECK_EQ(types.size(), 4);
+  const auto* input = types[0].as<TensorTypeNode>();
+  const auto* target = types[1].as<TensorTypeNode>();
+  const auto* weight = types[2].as<TensorTypeNode>();
+  const NLLLossAttrs* param = attrs.as<NLLLossAttrs>();
+  if (input == nullptr || target == nullptr || weight == nullptr) return false;
+  ICHECK(input->shape.size() - target->shape.size() == 1)
+      << "NLLLossRel: input should be one dimension larger than target, "
+      << "input shape = " << input->shape << ", "
+      << "target shape = " << target->shape;
+  ICHECK(weight->shape.size() == 1);

Review comment:
       Please add an error message.

##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -2973,6 +2973,34 @@ def cross_entropy_with_logits(predictions, targets):
     return _make.cross_entropy_with_logits(predictions, targets)
 
 
+def nll_loss(input, target, weight, reduction="mean", ignore_index=-100):
+    """Negative log likelihood loss.
+
+    Parameters
+    ----------
+    input : tvm.relay.Expr
+      The input.
+
+    target : tvm.relay.Expr
+      The target value of the input.
+
+    weight : tvm.relay.Expr
+      The weight of each target value.
+
+    reduction : string
+      The reduction method to apply to the output.

Review comment:
       Can you list the possible options for reduction.

##########
File path: src/relay/op/nn/nn.cc
##########
@@ -1091,6 +1092,60 @@ Accept logits.
 // Depth to space and space to depth
 TVM_REGISTER_NODE_TYPE(SubPixelAttrs);
 
+// relay.nn.nll_loss
+TVM_REGISTER_NODE_TYPE(NLLLossAttrs);
+
+bool NLLLossRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+                const TypeReporter& reporter) {
+  ICHECK_EQ(types.size(), 4);
+  const auto* input = types[0].as<TensorTypeNode>();
+  const auto* target = types[1].as<TensorTypeNode>();
+  const auto* weight = types[2].as<TensorTypeNode>();
+  const NLLLossAttrs* param = attrs.as<NLLLossAttrs>();
+  if (input == nullptr || target == nullptr || weight == nullptr) return false;
+  ICHECK(input->shape.size() - target->shape.size() == 1)
+      << "NLLLossRel: input should be one dimension larger than target, "
+      << "input shape = " << input->shape << ", "
+      << "target shape = " << target->shape;
+  ICHECK(weight->shape.size() == 1);
+  ICHECK(reporter->AssertEQ(input->shape[1], weight->shape[0]))
+      << "NLLLossRel: the second dimension of input should be the number of classes, "
+      << "which is the length of weight, "
+      << "input shape = " << input->shape << ", "
+      << "weight shape = " << weight->shape;
+  ICHECK(input->dtype == weight->dtype && input->dtype.is_float());
+  ICHECK(target->dtype.is_int());

Review comment:
       Please add error messages

##########
File path: python/tvm/topi/nn/loss.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name,unused-argument
+"""TVM operator negative log likelihood loss compute."""

Review comment:
       We may add other losses in the future, so maybe this should read something like "Loss functions definitions".




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-867291873


   @vinx13 @altanh Thank you for your help!
   
   > tag here is topi-level, sometimes we use it to identify a specific compute operation during schedule, otherwise we can leave it empty
   
   If I change the value of `tag` to an empty string, it will fail the check in `schedule_reduce`, which is:
   https://github.com/apache/tvm/blob/d0791d3db971a111826d96201bd1e4c9c0d531da/python/tvm/topi/x86/reduction.py#L84
   
   I'm not sure if I need to adjust somewhere else...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r649674949



##########
File path: tests/python/topi/python/test_topi_loss.py
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Test code for loss operators."""
+import numpy as np
+import pytest
+import tvm
+from tvm import te
+from tvm import topi
+import tvm.topi.testing
+
+import tvm.testing
+
+
+def verify_nll_loss(prediction_shape, reduction="mean", ignore_index=-100, dtype="float32"):
+    C = prediction_shape[1]
+    target_shape = prediction_shape[:1] + prediction_shape[2:]
+    predictions = te.placeholder(shape=prediction_shape, name="predictions", dtype=dtype)
+    targets = te.placeholder(shape=target_shape, name="targets", dtype="int32")
+    weights = te.placeholder(shape=(C,), name="weights", dtype=dtype)
+    nll_loss_result = topi.nn.nll_loss(
+        predictions, targets, weights, reduction, ignore_index
+    )
+
+    def check_device(target, dev):
+        print("Running on target: %s" % target)
+        with tvm.target.Target(target):
+            s = tvm.topi.testing.get_injective_schedule(target)(nll_loss_result)
+        fn = tvm.build(s, [predictions, targets, weights, nll_loss_result], target, name="nll_loss")
+        predictions_npy = np.random.uniform(size=prediction_shape).astype(dtype)
+        targets_npy = np.random.randint(0, C, target_shape).astype("int32")
+        weights_npy = np.random.uniform(size=(C,)).astype(dtype)
+        out_npy = tvm.topi.testing.nll_loss(predictions_npy,  targets_npy, weights_npy, reduction, ignore_index)
+        predictions_nd = tvm.nd.array(predictions_npy, dev)
+        targets_nd = tvm.nd.array(targets_npy, dev)
+        weights_nd = tvm.nd.array(weights_npy, dev)
+        out_nd = tvm.nd.array(np.empty(out_npy.shape).astype(nll_loss_result.dtype), dev)
+        fn(predictions_nd, targets_nd, weights_nd, out_nd)
+        out_topi = out_nd.asnumpy()
+        tvm.testing.assert_allclose(out_topi, out_npy)
+
+    for target, dev in tvm.testing.enabled_targets():
+        check_device(target, dev)
+
+
+@tvm.testing.uses_gpu
+def test_nll_loss():

Review comment:
       Refactored.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r644382553



##########
File path: tests/python/topi/python/test_topi_loss.py
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Test code for loss operators."""
+import numpy as np
+import pytest
+import tvm
+from tvm import te
+from tvm import topi
+import tvm.topi.testing
+
+import tvm.testing
+
+
+def verify_nll_loss(prediction_shape, reduction="mean", ignore_index=-100, dtype="float32"):
+    C = prediction_shape[1]
+    target_shape = prediction_shape[:1] + prediction_shape[2:]
+    predictions = te.placeholder(shape=prediction_shape, name="predictions", dtype=dtype)
+    targets = te.placeholder(shape=target_shape, name="targets", dtype="int32")
+    weights = te.placeholder(shape=(C,), name="weights", dtype=dtype)
+    nll_loss_result = topi.nn.nll_loss(
+        predictions, targets, weights, reduction, ignore_index
+    )
+
+    def check_device(target, dev):
+        print("Running on target: %s" % target)
+        with tvm.target.Target(target):
+            s = tvm.topi.testing.get_injective_schedule(target)(nll_loss_result)

Review comment:
       I'm not too familiar with the scheduling workflow, but is it right for this to be an injective schedule? cc @tkonolige 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r634036794



##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param input The input tensor.
+ * \param target The target tensor.
+ * \param weight A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& input, const Tensor& target, const Tensor& weight,
+                       std::string reduction = "mean", int ignore_index = -100,

Review comment:
       Yes, it's the default value used in pytorch, which is mainly a random negative number in my opinion. I added this default parameter so that the order of parameters can be the same of `aten::nll_loss`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-863697486


   @tkonolige I've updated the test. But there seems to be some error with the cuda target. Could you give me some help? Thank you! Part of the error message in the CI is listed below:
   
   >dev = cuda(0), target = 'cuda'
   >
   >
   >```python
   >    @tvm.testing.parametrize_targets
   >
   >    def test_nll_loss(dev, target):
   >
   >>       verify_nll_loss(dev, target, (10, 5))
   >```
   >
   >
   >tests/python/topi/python/test_topi_loss.py:60: 
   >
   >_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
   >
   >tests/python/topi/python/test_topi_loss.py:40: in verify_nll_loss
   >
   >    fn = tvm.build(s, [predictions, targets, weights, nll_loss_result], target, name="nll_loss")
   >
   >python/tvm/driver/build_module.py:353: in build
   >
   >    mod_host, mdev = _build_for_device(input_mod, tar, target_host)
   >
   >python/tvm/driver/build_module.py:177: in _build_for_device
   >
   >    mod_mixed = tvm.transform.Sequential(opt_mixed)(mod_mixed)
   >
   >python/tvm/ir/transform.py:161: in __call__
   >
   >    return _ffi_transform_api.RunPass(self, mod)
   >...
   >
   >E     Did you forget to bind?
   >
   >E       Variable `T_divide` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E       Variable `targets` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E       Variable `weights` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E       Variable `targets` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E       Variable `targets` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E       Variable `weights` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E       Variable `targets` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E       Variable `predictions` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E       Variable `targets` is directly accessed by host memory (it is not contained in a thread environment or in the function arguments.
   >
   >E     File "/workspace/src/tir/analysis/verify_memory.cc", line 202
   >
   >E   RuntimeError: Memory verification failed with the following errors:
   >
   >E   PrimFunc([predictions, targets, weights, T_divide]) attrs={"global_symbol": "nll_loss", "tir.noalias": (bool)1, "target": cuda -keys=cuda,gpu -max_num_threads=1024 -thread_warp_size=32} {
   >
   >E     // attr [nll_loss] storage_scope = "global"
   >
   >E     allocate nll_loss[float32 * 10]
   >
   >E     // attr [nll_loss_red] storage_scope = "global"
   >
   >E     allocate nll_loss_red[float32 * 1]
   >
   >E     // attr [nll_loss_red] storage_scope = "global"
   >
   >E     allocate nll_loss_red[float32 * 1]
   >
   >E     for (ax0, 0, 10) {
   >
   >E       nll_loss[ax0] = tir.if_then_else((targets[ax0] != -100), ((0f - predictions[((ax0*5) + targets[ax0])])*weights[targets[ax0]]), 0f)
   >
   >E     }
   >
   >E     nll_loss_red[0] = 0f
   >
   >E     for (k0, 0, 10) {
   >
   >E       nll_loss_red[0] = (nll_loss_red[0] + nll_loss[k0])
   >
   >E     }
   >
   >E     for (ax0, 0, 10) {
   >
   >E       nll_loss[ax0] = tir.if_then_else((targets[ax0] != -100), weights[targets[ax0]], 0f)
   >
   >E     }
   >
   >E     nll_loss_red[0] = 0f
   >
   >E     for (k0, 0, 10) {
   >
   >E       nll_loss_red[0] = (nll_loss_red[0] + nll_loss[k0])
   >
   >E     }
   >
   >E     T_divide[0] = (nll_loss_red[0]/nll_loss_red[0])
   >
   >E   }
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-843696836


   @tkonolige Sorry about the incomplete formula... I don't know what  I was thinking about yesterday :scream:


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-864760905


   @tkonolige Could you take another look? Thank you~


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-851021053


   @altanh @tkonolige Sorry for the late update. I've just added the test of `nll_loss` for relay, topi and pytorch frontend. Could you have another look?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r638225020



##########
File path: include/tvm/topi/nn.h
##########
@@ -642,6 +643,54 @@ inline tvm::te::Tensor batch_to_space_nd(const tvm::te::Tensor& data,
   out = strided_slice(out, begin_idx, end_idx, strides);
   return out;
 }
+
+/*!
+ * \brief Negative log likelihood loss.
+ *
+ * \param predictions The prediction tensor.
+ * \param targets The target tensor.
+ * \param weights A manual rescaling weight given to each class.
+ * \param reduction The reduction method to apply to the output.
+ * \param ignore_index The target value to ignore.
+ * \param name The name of the operation.
+ * \param tag The tag to mark the operation.
+ *
+ * \return A Tensor whose op member is the batch_to_space_nd operation
+ */
+inline Tensor nll_loss(const Tensor& predictions, const Tensor& targets, const Tensor& weights,
+                       std::string reduction = "mean", int ignore_index = -100,
+                       const std::string name = "nll_loss", const std::string tag = kBroadcast) {

Review comment:
       I see, that is confusing.. I'll get back to you on this soon




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] tkonolige commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r649334205



##########
File path: python/tvm/relay/op/nn/_nn.py
##########
@@ -886,6 +886,17 @@ def compute_cross_entropy_with_logits(attrs, inputs, out_dtype):
 reg.register_pattern("nn.cross_entropy_with_logits", OpPattern.OPAQUE)
 
 
+# nll_loss
+@reg.register_compute("nn.nll_loss")
+def compute_nll_loss(attrs, inputs, out_dtype):
+    predictions, targets, weights = inputs
+    return [topi.nn.nll_loss(predictions, targets, weights, attrs.reduction, attrs.ignore_index)]
+
+
+reg.register_reduce_schedule("nn.nll_loss")
+reg.register_pattern("nn.nll_loss", OpPattern.OPAQUE)

Review comment:
       ```suggestion
   reg.register_pattern("nn.nll_loss", OpPattern.OUT_ELEMWISE_FUSABLE)
   ```
   
   I believe you can fuse into the out here.

##########
File path: tests/python/topi/python/test_topi_loss.py
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Test code for loss operators."""
+import numpy as np
+import pytest
+import tvm
+from tvm import te
+from tvm import topi
+import tvm.topi.testing
+
+import tvm.testing
+
+
+def verify_nll_loss(prediction_shape, reduction="mean", ignore_index=-100, dtype="float32"):
+    C = prediction_shape[1]
+    target_shape = prediction_shape[:1] + prediction_shape[2:]
+    predictions = te.placeholder(shape=prediction_shape, name="predictions", dtype=dtype)
+    targets = te.placeholder(shape=target_shape, name="targets", dtype="int32")
+    weights = te.placeholder(shape=(C,), name="weights", dtype=dtype)
+    nll_loss_result = topi.nn.nll_loss(
+        predictions, targets, weights, reduction, ignore_index
+    )
+
+    def check_device(target, dev):
+        print("Running on target: %s" % target)
+        with tvm.target.Target(target):
+            s = tvm.topi.testing.get_injective_schedule(target)(nll_loss_result)
+        fn = tvm.build(s, [predictions, targets, weights, nll_loss_result], target, name="nll_loss")
+        predictions_npy = np.random.uniform(size=prediction_shape).astype(dtype)
+        targets_npy = np.random.randint(0, C, target_shape).astype("int32")
+        weights_npy = np.random.uniform(size=(C,)).astype(dtype)
+        out_npy = tvm.topi.testing.nll_loss(predictions_npy,  targets_npy, weights_npy, reduction, ignore_index)
+        predictions_nd = tvm.nd.array(predictions_npy, dev)
+        targets_nd = tvm.nd.array(targets_npy, dev)
+        weights_nd = tvm.nd.array(weights_npy, dev)
+        out_nd = tvm.nd.array(np.empty(out_npy.shape).astype(nll_loss_result.dtype), dev)
+        fn(predictions_nd, targets_nd, weights_nd, out_nd)
+        out_topi = out_nd.asnumpy()
+        tvm.testing.assert_allclose(out_topi, out_npy)
+
+    for target, dev in tvm.testing.enabled_targets():
+        check_device(target, dev)
+
+
+@tvm.testing.uses_gpu
+def test_nll_loss():

Review comment:
       Can you refactor this function to use tvm.testing.parametrize_targets (https://github.com/apache/tvm/blob/main/python/tvm/testing.py#L704)?

##########
File path: tests/python/topi/python/test_topi_loss.py
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Test code for loss operators."""
+import numpy as np
+import pytest
+import tvm
+from tvm import te
+from tvm import topi
+import tvm.topi.testing
+
+import tvm.testing
+
+
+def verify_nll_loss(prediction_shape, reduction="mean", ignore_index=-100, dtype="float32"):
+    C = prediction_shape[1]
+    target_shape = prediction_shape[:1] + prediction_shape[2:]
+    predictions = te.placeholder(shape=prediction_shape, name="predictions", dtype=dtype)
+    targets = te.placeholder(shape=target_shape, name="targets", dtype="int32")
+    weights = te.placeholder(shape=(C,), name="weights", dtype=dtype)
+    nll_loss_result = topi.nn.nll_loss(
+        predictions, targets, weights, reduction, ignore_index
+    )
+
+    def check_device(target, dev):
+        print("Running on target: %s" % target)
+        with tvm.target.Target(target):
+            s = tvm.topi.testing.get_injective_schedule(target)(nll_loss_result)

Review comment:
       Try
   ```suggestion
               s = tvm.te.create_schedule(nll_loss_result.op)
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin edited a comment on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin edited a comment on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-843696836


   @tkonolige Sorry about the incomplete formula... I don't know what  I was thinking about yesterday :scream:
   
   > are you planning on registering a gradient for this operator?
   
   @altanh Yes, I'm trying to  write one for this op. I will try to implement one like the cpu version of pytorch or onnxruntime). It may take some time since it's a little complex...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] vinx13 merged pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #8056:
URL: https://github.com/apache/tvm/pull/8056


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r638229466



##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -2973,6 +2973,40 @@ def cross_entropy_with_logits(predictions, targets):
     return _make.cross_entropy_with_logits(predictions, targets)
 
 
+def nll_loss(predictions, targets, weights, reduction="mean", ignore_index=-100):
+    """Negative log likelihood loss.
+
+    output{n, i_1, i_2, ..., i_k} = predictions{n, t, i_1, i_2, i_k}
+      where t = target{n, i_1, i_2, ..., i_k}
+
+    result = reduction(output)
+
+    Parameters
+    ----------
+    predictions : tvm.relay.Expr
+      The predictions.
+
+    targets : tvm.relay.Expr
+      The target value of each prediction.
+
+    weights : tvm.relay.Expr

Review comment:
       Hmm not sure, that's a good point- let's just keep the weights for now. As for not needing a gradient, currently there is no other way than just putting some dummy value. It might make sense for us to introduce a `stop_gradient` dummy op which cuts the gradient computation from going further at undifferentiable arguments (this can be a future PR). Thanks!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-845607944


   @altanh Thanks. I'll add the test soon. Could you also check the comments above relates to the optional weight, Diagnostics and the tag?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] tqchen commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-865972652


   cc @altanh @vinx13 it would be great if you can help take another look


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin edited a comment on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin edited a comment on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-845607944


   @altanh Thanks. I'll add the test soon. Could you also check the comments above relate to the optional weight, Diagnostics and the tag?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] zhuzilin commented on pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
zhuzilin commented on pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#issuecomment-864760905


   @tkonolige Could you take another look? Thank you~


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] altanh commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r644382553



##########
File path: tests/python/topi/python/test_topi_loss.py
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Test code for loss operators."""
+import numpy as np
+import pytest
+import tvm
+from tvm import te
+from tvm import topi
+import tvm.topi.testing
+
+import tvm.testing
+
+
+def verify_nll_loss(prediction_shape, reduction="mean", ignore_index=-100, dtype="float32"):
+    C = prediction_shape[1]
+    target_shape = prediction_shape[:1] + prediction_shape[2:]
+    predictions = te.placeholder(shape=prediction_shape, name="predictions", dtype=dtype)
+    targets = te.placeholder(shape=target_shape, name="targets", dtype="int32")
+    weights = te.placeholder(shape=(C,), name="weights", dtype=dtype)
+    nll_loss_result = topi.nn.nll_loss(
+        predictions, targets, weights, reduction, ignore_index
+    )
+
+    def check_device(target, dev):
+        print("Running on target: %s" % target)
+        with tvm.target.Target(target):
+            s = tvm.topi.testing.get_injective_schedule(target)(nll_loss_result)

Review comment:
       I'm not too familiar with the scheduling workflow, but is it right for this to be an injective schedule? cc @tkonolige 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [tvm] vinx13 commented on a change in pull request #8056: [Relay, TOPI] Add negative log likelihood loss (nll_loss) op

Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #8056:
URL: https://github.com/apache/tvm/pull/8056#discussion_r657370864



##########
File path: tests/python/topi/python/test_topi_loss.py
##########
@@ -0,0 +1,69 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Test code for loss operators."""
+import numpy as np
+import pytest
+import tvm
+from tvm import te
+from tvm import topi
+import tvm.topi.testing
+
+import tvm.testing
+
+
+def verify_nll_loss(
+    dev, target, prediction_shape, reduction="mean", ignore_index=-100, dtype="float32"
+):
+    C = prediction_shape[1]
+    target_shape = prediction_shape[:1] + prediction_shape[2:]
+    predictions = te.placeholder(shape=prediction_shape, name="predictions", dtype=dtype)
+    targets = te.placeholder(shape=target_shape, name="targets", dtype="int32")
+    weights = te.placeholder(shape=(C,), name="weights", dtype=dtype)
+    nll_loss_result = topi.nn.nll_loss(predictions, targets, weights, reduction, ignore_index)
+
+    with tvm.target.Target(target):
+        s = tvm.te.create_schedule(nll_loss_result.op)

Review comment:
       default schedule is used here which caused ci errors
   ```suggestion
           fschedule = tvm.topi.testing.get_reduce_schedule(target)
           s = fschedule([nll_loss_result])
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org