CPP implementation of L2Norm and LRN ops by PariksheetPinjari909 · Pull Request #1157 · apache/tvm

PariksheetPinjari909 · 2018-05-11T09:50:30Z

This PR has CPP implementation of LRN and L2Norm. It also redirect LRN and L2Norm python ops to CPP ops.

tqchen · 2018-05-11T17:01:27Z

@sxjscience can you please do a round of code review?

PariksheetPinjari909 · 2018-05-30T09:03:37Z

I have added nnvm frontend support for lrn and l2norm ops. Please review

PariksheetPinjari909 · 2018-06-04T12:11:24Z

@tqchen, I am facing a build error only in the i386 environment with an unrelated source code.
make: *** No rule to make target 'topi/include/topi/nn/scale.h', needed by 'build/topi/topi.o'. Stop. ///scale.h is not related to this pull request.

Could you please help for a clean build to see whether it is an environment issue?

tqchen · 2018-06-04T16:38:23Z

@kazum @merrymercy can you help review this PR?

kazum · 2018-06-06T09:03:54Z

+    DMLC_DECLARE_FIELD(axis)
+      .describe("input data layout channel axis");
+    DMLC_DECLARE_FIELD(alpha)
+      .describe("alpha constant.");


The scaling parameter.

kazum · 2018-06-06T09:04:12Z

+    DMLC_DECLARE_FIELD(alpha)
+      .describe("alpha constant.");
+    DMLC_DECLARE_FIELD(beta)
+      .describe("beta constant.");


The exponent number.

kazum · 2018-06-06T09:04:29Z

+    DMLC_DECLARE_FIELD(beta)
+      .describe("beta constant.");
+    DMLC_DECLARE_FIELD(bias)
+      .describe("bias constant.");


The offset parameter.

kazum · 2018-06-06T09:05:42Z


-import topi
 import tvm
+import topi


Do we need this change?

Lint is suggesting this change.

kazum · 2018-06-06T09:06:13Z

+            offset to avoid dividing by 0. constant value
+
+        alpha : float
+            contant valie


constant value

kazum · 2018-06-06T09:08:20Z

+  auto lrn = outs[0];
+  auto sqr_sum_up = lrn->op->InputTensors()[1];
+  auto sqr_sum = sqr_sum_up->op->InputTensors()[0];
+  auto set_pad = sqr_sum->op->InputTensors()[0];


I think it's better to use concrete types:
https://github.com/dmlc/tvm/blob/master/docs/contribute/code_guide.rst#c-code-styles

kazum · 2018-06-06T09:09:40Z

 *  Copyright (c) 2017 by Contributors
 * \brief NN op constructions
- * \file topi/nn.h
+ * \file


* \file nn.h?

kazum · 2018-06-06T09:11:16Z

+using namespace tvm;
+
+/*!
+* \brief L2 normalization inference operator 


Remove a trailing space.

kazum · 2018-06-06T09:13:59Z

+                  std::string tag = kBroadcast) {
+  CHECK_EQ(data->shape.size(), 4) << "LRN requires 4-D input";
+  assert(size % 2 == 1);
+  assert(axis == 1 || axis == 3);


I think we should use CHECK_* macros here because assert() can be compiled out when we define NDEBUG.

PariksheetPinjari909 · 2018-06-07T06:21:01Z

@tqchen I am facing the same error again
make: *** No rule to make target 'topi/include/topi/nn/scale.h', needed by 'build/topi/topi.o'. Stop.

tqchen · 2018-06-08T04:34:34Z

  }
 };

+struct LrnParam : public dmlc::Parameter<LrnParam> {


tqchen · 2018-06-08T04:35:22Z

+DMLC_REGISTER_PARAMETER(L2normParam);
+
+inline bool L2normInferShape(const nnvm::NodeAttrs& attrs,
+                          std::vector<TShape>* in_shape,


tqchen · 2018-06-08T04:37:21Z

+  return true;
+}
+
+NNVM_REGISTER_OP(l2norm)


l2norm not a typical operator, a typical version os numpy.lingalg.norm, so I would recommend we do not add registration and support proper norm in a separate PR

Okay, it is good idea to have generalized norm operation. I will remove L2Norm from this PR and will raise another PR to have generalized norm operation.

Sorry, I was confused by the name of the API, the current API is l2_normalize which performs the normalization, instead of calculating the norm.

Let us make the name clear in both TOPI and nnvm.
c.f. related tensorflow API https://www.tensorflow.org/api_docs/python/tf/nn/l2_normalize

L2 norm operation can be used to perform l2 normalization. If we are planning to add generalized norm op, we can use the same to compute l2 normalization.

tqchen · 2018-06-08T04:37:42Z

+                sqr_sum[i, j, k, l] = sum(a_np[i, j, k, sum_start:sum_end] * \
+                                          a_np[i, j, k, sum_start:sum_end])
+
+        for i in range(axis0):


use broadcasting semantics

@tqchen , I have tried using broadcast operations further on this changes, but here the sum is doing based on window size and the window move across the axis. could you please elaborate your suggestion

OK, never mind, it is fine to keep this one as for loop

tqchen · 2018-06-08T04:38:06Z

+*
+* \return A schedule for the given ops.
+*/
+inline Schedule schedule_l2norm(const Target &target, const Array<Tensor>& outs) {


remove L2 norm for now and can do that in separate PR, support norm later.

kazum · 2018-06-08T05:33:08Z

    inputs = [('x', (1, 3, 28, 28), x)]
    helper(y, inputs, dtype, forward)

+def verify_lrn(n, c, h, w, size, axis, bias, alpha, beta):


def verify_lrn(ishape, size, axis, bias, alpha, beta) and using ishape instead of dshape looks simpler.

kazum · 2018-06-08T05:33:25Z

+            offset to avoid dividing by 0. constant value
+
+        alpha : float
+            contant value


kazum · 2018-06-08T05:33:58Z

+        radius = size // 2
+        sqr_sum = np.zeros(shape=a_np.shape).astype(a_np.dtype)
+        sqr_sum_up = np.zeros(shape=a_np.shape).astype(a_np.dtype)
+        lrn_out = np.zeros(shape=a_np.shape).astype(a_np.dtype)


This line can be removed.

kazum · 2018-06-08T05:34:44Z

+        out_np = lrn_python(x_np, size, axis, bias, alpha, beta)
+        np.testing.assert_allclose(out.asnumpy(), out_np, atol=1e-5, rtol=1e-5)
+
+def verify_l2norm(batch, channel, height, width, eps, axis):


def verify_l2norm(ishape, eps, axis) looks simpler.

kazum · 2018-06-08T05:35:32Z

+                  std::string tag = kBroadcast) {
+  CHECK_EQ(data->shape.size(), 4) << "LRN requires 4-D input";
+  CHECK_EQ(size % 2, 1) << "size should be odd number";
+  CHECK_EQ((axis - 1) && (axis - 3), 0) << "axis should be 1 or 3 for NCHW and NHWC";


CHECK(axis == 1 || axis == 3)

kazum · 2018-06-08T05:38:51Z

+    l2norm_out : np.ndarray
+        4-D with shape [batch, out_channel, out_height, out_width]
+    """
+    batch, axis1, axis2, axis3 = a_np.shape


batch = a_np.shape[0] looks simpler? We don't need axis[1-3].

kazum · 2018-06-08T05:39:06Z

+    batch, axis1, axis2, axis3 = a_np.shape
+    sqr_sum = np.zeros(shape=(batch,)).astype(a_np.dtype)
+    sqrt_sum = np.zeros(shape=(batch,)).astype(a_np.dtype)
+    l2norm_out = np.zeros(shape=a_np.shape).astype(a_np.dtype)


Can be removed.

kazum · 2018-06-08T05:39:54Z

+    sqrt_sum = np.sqrt(np.maximum(np.broadcast_to(sqr_sum, a_np.shape), eps))
+    return np.divide(a_np, sqrt_sum)
+
+def verify_l2norm(n, c, h, w, eps, axis=None):


I'd suggest def verify_l2norm(shape, eps, axis=None).

kazum · 2018-06-08T05:40:12Z

@@ -0,0 +1,101 @@
+"""Test code for LRN"""
+import os


Can be removed.

kazum · 2018-06-08T05:41:02Z

+    radius = size // 2
+    sqr_sum = np.zeros(shape=a_np.shape).astype(a_np.dtype)
+    sqr_sum_up = np.zeros(shape=a_np.shape).astype(a_np.dtype)
+    lrn_out = np.zeros(shape=a_np.shape).astype(a_np.dtype)


tqchen · 2018-06-08T18:24:18Z

+  return true;
+}
+
+NNVM_REGISTER_OP(l2norm)


Sorry, I was confused by the name of the API, the current API is l2_normalize which performs the normalization, instead of calculating the norm.

Let us make the name clear in both TOPI and nnvm.
c.f. related tensorflow API https://www.tensorflow.org/api_docs/python/tf/nn/l2_normalize

tqchen · 2018-06-11T23:59:25Z

OK, please fix the comments, remove l2norm or add l2_normalize to this PR and let us aim to prioritize and bring this in. This code review has been hanging for a bit long

tqchen · 2018-06-15T04:17:13Z

@PariksheetPinjari909 can you act on the comments? Rebase is needed

PariksheetPinjari909 · 2018-06-15T05:02:10Z

@tqchen Thanks, i was about to commit, then i saw rebase is needed. I have made the l2normalize naming to avoid future confusion. Pls review.

PariksheetPinjari909 · 2018-06-15T05:02:47Z

@kazum Pls review

kevinthesun · 2018-06-16T04:46:10Z

+.set_num_outputs(1)
+.set_attr<FInferShape>("FInferShape", L2normalizeInferShape)
+.set_attr<FInferType>("FInferType", ElemwiseType<1, 1>)
+.set_support_level(1);


We need FCorrectLayout attribute to for correct layout pass.

tqchen · 2018-06-16T04:50:06Z

+
+reg.register_pattern("lrn", OpPattern.OUT_ELEMWISE_FUSABLE)
+
+@reg.register_compute("l2normalize")


use l2_normalize(with underscore), to be consistent with tensorflow API

tqchen · 2018-06-16T04:51:07Z

+    with tvm.target.create(target):
+        return topi.generic.schedule_lrn(outs)
+
+reg.register_pattern("lrn", OpPattern.OUT_ELEMWISE_FUSABLE)


can we confirm if lrn is OUT_ELEMWISE_FUSABLE. We need to add a testcase, lrn followed by relu, and confirm if the test pass on GPU

tqchen · 2018-06-16T04:51:54Z

+*
+* \return A Tensor whose op member is the l2 normalization operation
+*/
+inline Tensor l2normalize_instance(const Tensor& data,


l2_normalize

what does instance mean in here?

Instance name was given with respect to mxnet l2_normalize function, but now we are supporting l2_normalize in all axes so no need to keep the instance name. I will remove it. Thanks for pointing out.

tqchen · 2018-06-16T04:52:57Z

+    with tvm.target.create(target):
+        return topi.generic.schedule_l2normalize(outs)
+
+reg.register_pattern("l2normalize", OpPattern.OUT_ELEMWISE_FUSABLE)


if we want to mark it as OUT_ELEMWISE_FUSABLE, confirm with a testcase of op + elemwise operator so that it generate testcase for used ops

kazum · 2018-06-17T19:28:08Z

+        l2normalize_out : np.ndarray
+            4-D with shape [batch, out_channel, out_height, out_width]
+        """
+        batch = a_np.shape[0]


This line can be removed.

kazum · 2018-06-17T19:28:45Z

-    sqr_sum = np.zeros(shape=(batch,)).astype(a_np.dtype)
-    sqrt_sum = np.zeros(shape=(batch,)).astype(a_np.dtype)
-    l2norm_out = np.zeros(shape=a_np.shape).astype(a_np.dtype)
+    batch = a_np.shape[0]


kazum · 2018-06-17T19:29:14Z

+    l2normalize_out : np.ndarray
+        4-D with shape [batch, out_channel, out_height, out_width]
+    """
+    batch = a_np.shape[0]


kazum · 2018-06-21T09:15:40Z

+
+NNVM_REGISTER_OP(lrn)
+.describe(R"code(LRN layer)code" NNVM_ADD_FILELINE)
+.add_argument("data", "4D Tesndor", "Input data.")


"4D Tensor"

kazum · 2018-06-21T09:15:56Z

+
+NNVM_REGISTER_OP(l2_normalize)
+.describe(R"code(L2NORMALIZE layer)code" NNVM_ADD_FILELINE)
+.add_argument("data", "4D Tesndor", "Input data.")


"4D Tensor"

kazum · 2018-06-21T09:21:52Z

+        axis0, axis1, axis2, axis3 = a_np.shape
+        radius = size // 2
+        sqr_sum = np.zeros(shape=a_np.shape).astype(a_np.dtype)
+        def sum_dot_values(i, j, k, l):


from itertools import product for i, j, k, l in product(*[range(_axis) for _axis in a_np.shape]):

and we can remove the nested loop below. I think this cleanup is matter of taste, though.

Yes, it looks nicer now. Thanks

tqchen · 2018-06-21T17:01:06Z

+    dtype = "float32"
+    x_np = np.random.uniform(size=ishape).astype(dtype)
+
+    def l2_normalize_python(a_np, eps, axis=None):


move this function to topi.testing.

tqchen · 2018-06-21T17:01:29Z

+    dtype = "float32"
+    x_np = np.random.uniform(size=ishape).astype(dtype)
+
+    def lrn_python(a_np, size, axis, bias, alpha, beta):


move this function to topi.testing

tqchen · 2018-06-21T17:02:35Z

Some final comments and should be approved from my side, await @kazum 's comment

kazum

Looks good from my side, thanks!

tqchen · 2018-06-21T20:26:56Z

@PariksheetPinjari909 please act on my final comments

PariksheetPinjari909 · 2018-06-22T17:00:41Z

@tqchen all reviews are handled now. Pls check.

tqchen · 2018-06-22T17:06:49Z

Thanks! This is merged!

PariksheetPinjari909 force-pushed the cpp_lrn_l2norm branch from 7ceb221 to d351033 Compare May 30, 2018 08:39

This was referenced May 30, 2018

[OP] Support LRN #1185

Closed

[FRONTEND][MXNET] L2Normalization is not supported in nnvm #1223

Closed

PariksheetPinjari909 force-pushed the cpp_lrn_l2norm branch from 2070956 to eca71e4 Compare June 4, 2018 05:15

tqchen added the status: need review label Jun 4, 2018

rajh619 mentioned this pull request Jun 5, 2018

[ONNX][OP] Support GEMM #1231

Closed

PariksheetPinjari909 force-pushed the cpp_lrn_l2norm branch from eca71e4 to 2e11f32 Compare June 5, 2018 13:20

kazum requested changes Jun 6, 2018

View reviewed changes

PariksheetPinjari909 force-pushed the cpp_lrn_l2norm branch from 2e11f32 to 776989a Compare June 7, 2018 06:11

tqchen requested changes Jun 8, 2018

View reviewed changes

kazum requested changes Jun 8, 2018

View reviewed changes

tqchen requested changes Jun 8, 2018

View reviewed changes

tqchen added status: review in progress and removed status: need review labels Jun 11, 2018

tqchen added the status: need update need update based on feedbacks label Jun 12, 2018

kevinthesun mentioned this pull request Jun 14, 2018

[OP] SSD gluon-cv model support #1269

Closed

4 tasks

PariksheetPinjari909 force-pushed the cpp_lrn_l2norm branch from 776989a to fa22f79 Compare June 15, 2018 04:35

kevinthesun mentioned this pull request Jun 16, 2018

SSD support in NNVM #1214

Merged

kevinthesun reviewed Jun 16, 2018

View reviewed changes

tqchen requested changes Jun 16, 2018

View reviewed changes

kazum reviewed Jun 17, 2018

View reviewed changes

PariksheetPinjari909 force-pushed the cpp_lrn_l2norm branch from f60cb88 to 91b6025 Compare June 21, 2018 04:29

kazum reviewed Jun 21, 2018

View reviewed changes

PariksheetPinjari909 force-pushed the cpp_lrn_l2norm branch from 91b6025 to e91e6d1 Compare June 21, 2018 11:01

tqchen requested changes Jun 21, 2018

View reviewed changes

kazum approved these changes Jun 21, 2018

View reviewed changes

PariksheetPinjari909 added 12 commits June 22, 2018 08:55

CPP implementation of L2Norm and LRN ops

0d39f88

Sanity check issue fixed

d9e5e77

nnvm support for lrn and l2norm ops added

9dfab01

lint error fixed

620f9ef

Build check

8886fb9

build recheck

c2dcc60

Review comments updated

c41f998

Review comments reworked

5a44f63

Review comments addressed

88a12f5

Consistent l2_normalize name

be94a2e

Modified lrn_python function

e5bc18b

Moved lrn_python and l2_normalize_python to topi.testing

c2ca9c2

PariksheetPinjari909 force-pushed the cpp_lrn_l2norm branch from e91e6d1 to c2ca9c2 Compare June 22, 2018 04:29

tqchen approved these changes Jun 22, 2018

View reviewed changes

tqchen merged commit e0e0a23 into apache:master Jun 22, 2018

tqchen added status: accepted and removed status: need update need update based on feedbacks status: review in progress labels Jun 22, 2018

tqchen pushed a commit to tqchen/tvm that referenced this pull request Jul 6, 2018

CPP implementation of L2Norm and LRN ops (apache#1157)

5938b66

mnuyens pushed a commit to mnuyens/tvm that referenced this pull request Jul 10, 2018

CPP implementation of L2Norm and LRN ops (apache#1157)

fb88b74

sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018

CPP implementation of L2Norm and LRN ops (apache#1157)

00fd8dd


		reg.register_pattern("lrn", OpPattern.OUT_ELEMWISE_FUSABLE)

		@reg.register_compute("l2normalize")

Uh oh!

Conversation

PariksheetPinjari909 commented May 11, 2018

Uh oh!

tqchen commented May 11, 2018

Uh oh!

PariksheetPinjari909 commented May 30, 2018

Uh oh!

PariksheetPinjari909 commented Jun 4, 2018

Uh oh!

tqchen commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PariksheetPinjari909 commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tqchen Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tqchen Jun 8, 2018 •

edited

Loading

tqchen Jun 8, 2018 •

edited

Loading