[TOPI][RELAY] Add op Size #3094

yongwww · 2019-04-25T18:44:17Z

Add op Size. Size is used in numpy, TensorFlow and MXNet, is needed for tf object detection models like mask-rcnn.

@zhiics @kevinthesun @icemelon9 @jroesch @Laurawly

yzhliu

Also please fix the code style: 2-space indent for c++

yzhliu · 2019-04-26T05:54:29Z

src/relay/op/tensor/unary.cc

+    CHECK(tt != nullptr);
+    const auto* param = attrs.as<SizeAttrs>();
+    CHECK(param != nullptr);
+    reporter->Assign(types[1], TensorTypeNode::make({1}, param->dtype));


looks like type of size in numpy is scalar?

Yeah, both numpy.size and tf.size are scalar

but {1} here is tensor? I think we need to keep it the same as np

good catch, sure I'll update the code

zhiics

Some nits

zhiics · 2019-04-26T07:22:16Z

topi/include/topi/transform.h

+                   const std::string name = "size",
+                   const std::string tag = kInjective) {
+  int ndim = static_cast<int>(src->shape.size());
+  Array<Expr> out_size;


use out_size{1} directly?

zhiics · 2019-04-26T07:42:32Z

topi/include/topi/transform.h

+ * \return Tensor of input shape.
+ */
+inline Tensor size(const Tensor& src,
+                   Type dtype,


const Type& ?

zhiics · 2019-04-26T07:43:03Z

topi/src/topi.cc

@@ -300,6 +300,11 @@ TVM_REGISTER_GLOBAL("topi.shape")
  *rv = shape(args[0], args[1]);
 });

+TVM_REGISTER_GLOBAL("topi.size")
+.set_body([](TVMArgs args, TVMRetValue *rv) {
+*rv = size(args[0], args[1]);


2 space indent

zhiics · 2019-04-26T07:43:46Z

topi/tests/python/test_topi_transform.py

+            return
+        tvm_input = tvm.nd.array(input, ctx=ctx)
+        tvm_output = tvm.nd.empty((1,), ctx=ctx, dtype=B.dtype)
+        print("Running on target: %s" % device)


remove or log?

simply use print here to make it consistent with all the other tests in topi. All test cases are using print.

zhiics · 2019-04-26T07:52:40Z

topi/include/topi/transform.h

+ */
+inline Tensor size(const Tensor& src,
+                   Type dtype,
+                   const std::string name = "size",


either std::string or const std::string& ?

icemelon · 2019-04-26T05:26:05Z

src/relay/op/tensor/unary.cc

+.set_attr<TOpPattern>("TOpPattern", kInjective)
+.set_attr<FInferCorrectLayout>("FInferCorrectLayout",
+ElemwiseArbitraryLayout)
+.set_support_level(3)


Change support level to 10.

Laurawly · 2019-04-26T20:54:33Z

tests/python/relay/test_op_level3.py

@@ -607,6 +607,24 @@ def verify_gather_nd(xshape, yshape, y_data):
    verify_gather_nd((2, 2, 2), (2, 2), [[0, 1], [1, 0]])


+def test_size():


Move the test to test_op_level10.py

tqchen · 2019-04-27T16:46:26Z

We need to debate a bit about the API. given that numpy's size is ndarray.size, we may not want to directly use relay.size as the operator name. If it is a contrib op, consider something like relay.contrib.array_size

icemelon · 2019-05-24T22:56:54Z

What is the status of this PR? @yongwww @tqchen

yongwww · 2019-05-25T16:56:40Z

@icemelon9 need to address comments, especially the returned scalar representation. I am working on it.

jroesch · 2019-05-31T09:17:39Z

We need to debate a bit about the API. given that numpy's size is ndarray.size, we may not want to directly use relay.size as the operator name. If it is a contrib op, consider something like relay.contrib.array_size

I think we should name tensor_size given we only talk about Tensors in Relay and not arrays.

tqchen · 2019-05-31T17:45:13Z

@jroesch the naming convention is a bit debatable, given numpy use ndarray and we also use NDArray as our storage type, my guess is that array_size is fine if we want to be relatively consistent with numpy

yongwww · 2019-06-05T19:31:53Z

@tqchen @jroesch both tensor_size and array_size make sense to me. TensorFlow and NumPy are using size. In PyTorch, the corresponding name is numel(number of elements of tensor. size is used to get the shape of the tensor). How about relay.contrib.numel? another candidate relay.contrib.num_element.

tqchen · 2019-06-06T19:07:46Z

relay.contrib.num_elements seems to be a clear name

yongwww · 2019-06-14T04:49:18Z

@yzhliu @jroesch @zhiics @icemelon9 @tqchen @kevinthesun have incorporated comments except for the scalar representation. I tried to add scalar support in my own branch. But failed to run on graph runtime since the schedule was mutated in Halide improperly. Filed an issue.

jroesch · 2019-06-14T22:20:28Z

Can you rebase?

jroesch

Can you just address Zhi's comments?

yongwww · 2019-06-14T23:43:28Z

@jroesch @zhiics rebased and fixed zhi's comments.

a1010428282 · 2019-07-15T05:25:58Z

I wonder if this commit will continue？

tqchen · 2019-07-16T17:34:29Z

@yongwww please rebase, rename to ndarray_size and let us merge this

yongwww · 2019-07-17T01:41:11Z

@tqchen renamed. Pls take a look

tqchen · 2019-07-17T20:20:15Z

Please rename the topi api name as well.

yongwww · 2019-07-18T16:22:24Z

@tqchen updated. Seems CI hangs

yongwww requested review from Huyuwei and Laurawly as code owners April 25, 2019 18:44

yzhliu reviewed Apr 26, 2019

View reviewed changes

zhiics reviewed Apr 26, 2019

View reviewed changes

icemelon reviewed Apr 26, 2019

View reviewed changes

Laurawly reviewed Apr 26, 2019

View reviewed changes

yzhliu added the status: need update need update based on feedbacks label Apr 29, 2019

kevinthesun self-assigned this May 24, 2019

yongwww force-pushed the master branch 4 times, most recently from 69ca070 to 48d69ac Compare June 13, 2019 16:59

jroesch approved these changes Jun 14, 2019

View reviewed changes

yongwww force-pushed the master branch from 48d69ac to 9120ce9 Compare June 14, 2019 23:41

yongwww and others added 3 commits July 16, 2019 16:02

[TOPI][RELAY] Add op Size

fa82602

use contrib.num_elements

7267d00

rebase, fix comments

25de6b3

yongwww force-pushed the master branch from 9120ce9 to 721e0a8 Compare July 16, 2019 23:34

yongwww force-pushed the master branch from 721e0a8 to 6018bca Compare July 16, 2019 23:45

yongwww force-pushed the master branch from 6018bca to e7c494b Compare July 17, 2019 21:40

rename to ndarray_size

e017ed3

yongwww force-pushed the master branch from e7c494b to e017ed3 Compare July 19, 2019 16:09

tqchen merged commit 313bc9d into apache:master Jul 19, 2019

tqchen added status: accepted and removed status: need update need update based on feedbacks labels Jul 19, 2019

wweic pushed a commit to wweic/tvm that referenced this pull request Aug 9, 2019

[TOPI][RELAY] Add op Size (apache#3094)

034524c

wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 6, 2019

[TOPI][RELAY] Add op Size (apache#3094)

6c1979f

tqchen unassigned kevinthesun Nov 4, 2019

tqchen mentioned this pull request Nov 8, 2019

[RELEASE][DRAFT] TVM v0.6 Release candidate #4259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TOPI][RELAY] Add op Size #3094

[TOPI][RELAY] Add op Size #3094

yongwww commented Apr 25, 2019 •

edited

Loading

yzhliu left a comment

yzhliu Apr 26, 2019

yongwww Apr 26, 2019

yzhliu Apr 26, 2019

yongwww Apr 26, 2019

zhiics left a comment

zhiics Apr 26, 2019

zhiics Apr 26, 2019

zhiics Apr 26, 2019

zhiics Apr 26, 2019

yongwww Jun 14, 2019

zhiics Apr 26, 2019

icemelon Apr 26, 2019

Laurawly Apr 26, 2019

tqchen commented Apr 27, 2019

icemelon commented May 24, 2019

yongwww commented May 25, 2019

jroesch commented May 31, 2019

tqchen commented May 31, 2019

yongwww commented Jun 5, 2019

tqchen commented Jun 6, 2019 •

edited

Loading

yongwww commented Jun 14, 2019 •

edited

Loading

jroesch commented Jun 14, 2019

jroesch left a comment

yongwww commented Jun 14, 2019

a1010428282 commented Jul 15, 2019

tqchen commented Jul 16, 2019

yongwww commented Jul 17, 2019

tqchen commented Jul 17, 2019

yongwww commented Jul 18, 2019

		@@ -607,6 +607,24 @@ def verify_gather_nd(xshape, yshape, y_data):
		verify_gather_nd((2, 2, 2), (2, 2), [[0, 1], [1, 0]])


		def test_size():

[TOPI][RELAY] Add op Size #3094

[TOPI][RELAY] Add op Size #3094

Conversation

yongwww commented Apr 25, 2019 • edited Loading

yzhliu left a 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

zhiics left a 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 Apr 27, 2019

icemelon commented May 24, 2019

yongwww commented May 25, 2019

jroesch commented May 31, 2019

tqchen commented May 31, 2019

yongwww commented Jun 5, 2019

tqchen commented Jun 6, 2019 • edited Loading

yongwww commented Jun 14, 2019 • edited Loading

jroesch commented Jun 14, 2019

jroesch left a comment

Choose a reason for hiding this comment

yongwww commented Jun 14, 2019

a1010428282 commented Jul 15, 2019

tqchen commented Jul 16, 2019

yongwww commented Jul 17, 2019

tqchen commented Jul 17, 2019

yongwww commented Jul 18, 2019

yongwww commented Apr 25, 2019 •

edited

Loading

tqchen commented Jun 6, 2019 •

edited

Loading

yongwww commented Jun 14, 2019 •

edited

Loading