CPP implementation of L2Norm and LRN ops #1157

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

nnvm/include/nnvm/top/nn.h

+    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

nnvm/include/nnvm/top/nn.h

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


The exponent number.

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

nnvm/include/nnvm/top/nn.h

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


The offset parameter.

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

nnvm/python/nnvm/top/vision.py

 import tvm
+import topi


Do we need this change?

Lint is suggesting this change.

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

nnvm/tests/python/compiler/test_top_level1.py

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


constant value

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

topi/include/topi/cuda/nn.h

+  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

topi/include/topi/nn.h

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


* \file nn.h?

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

topi/include/topi/nn/l2_norm.h

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


Remove a trailing space.

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

topi/include/topi/nn/local_response_norm.h

+                  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

nnvm/include/nnvm/top/nn.h

@@ -313,6 +313,41 @@ struct LayoutTransformParam : public dmlc::Parameter<LayoutTransformParam> {
  }
 };

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


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

nnvm/src/top/nn/nn.cc

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


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

nnvm/src/top/nn/nn.cc

+  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

nnvm/tests/python/compiler/test_top_level1.py

+                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

topi/include/topi/cuda/normalization.h

+*
+* \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

nnvm/tests/python/compiler/test_top_level1.py

@@ -365,6 +365,129 @@ def forward(x):
    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

nnvm/tests/python/compiler/test_top_level1.py

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


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

nnvm/tests/python/compiler/test_top_level1.py

+        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

nnvm/tests/python/compiler/test_top_level1.py

+        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

topi/include/topi/nn/local_response_norm.h

+                  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

topi/tests/python_cpp/test_topi_l2norm.py

+    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

topi/tests/python_cpp/test_topi_l2norm.py

+    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

topi/tests/python_cpp/test_topi_l2norm.py

+    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

topi/tests/python_cpp/test_topi_lrn.py

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


Can be removed.

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

topi/tests/python_cpp/test_topi_lrn.py

+    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

nnvm/src/top/nn/nn.cc

+  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

nnvm/src/top/nn/nn.cc

+.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

nnvm/python/nnvm/top/nn.py

+
+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

nnvm/python/nnvm/top/nn.py

+    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

topi/include/topi/nn/l2_normalize.h

+*
+* \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

nnvm/python/nnvm/top/nn.py

+    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

nnvm/tests/python/compiler/test_top_level1.py

+        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

topi/tests/python/test_topi_l2norm.py

-    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

topi/tests/python_cpp/test_topi_l2norm.py

+    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/src/top/nn/nn.cc

+
+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/src/top/nn/nn.cc

+
+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

nnvm/tests/python/compiler/test_top_level1.py

+        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

nnvm/tests/python/compiler/test_top_level1.py

+    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

nnvm/tests/python/compiler/test_top_level1.py

+    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

grwlf pushed a commit to grwlf/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")

CPP implementation of L2Norm and LRN ops #1157

CPP implementation of L2Norm and LRN ops #1157

Conversation

PariksheetPinjari909 commented May 11, 2018

tqchen commented May 11, 2018

PariksheetPinjari909 commented May 30, 2018

PariksheetPinjari909 commented Jun 4, 2018

tqchen commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PariksheetPinjari909 commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen Jun 8, 2018 • edited

Choose a reason for hiding this comment

tqchen commented Jun 11, 2018

tqchen commented Jun 15, 2018

PariksheetPinjari909 commented Jun 15, 2018

PariksheetPinjari909 commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jun 21, 2018

kazum left a comment

Choose a reason for hiding this comment

tqchen commented Jun 21, 2018

PariksheetPinjari909 commented Jun 22, 2018

tqchen commented Jun 22, 2018

tqchen Jun 8, 2018 •

edited

tqchen Jun 8, 2018 •

edited