[TOPI][RELAY][OP] add op crop_and_resize #4417

yongwww · 2019-11-24T23:43:10Z

Add crop_and_resize op into topi and relay and TensorFlow frontend.

Some of the reasons why we need crop_and_resize are as below:

CropAndResize is being used in TensorFlow frequently in both training and inference, 'almost' all models (such as ssd, maskrcnn, etc.) in tensorflow object detection zoo are using this op.
It is possible to use strided_slice, resize and concatenate to compose crop_and_resize for some cases, but the composition is not able to cover all cases, for example, when extrapolation_value is used, boxes and/or box_indices are not list, etc.
Most importantly, using strided_slice, resize and concatenate to build CropAndResize will introduce a significant number of ops in the converted relay ir when there are a lot of boxes as input, it does happen in models like ssd in tensorflow object detection model zoo.

@kevinthesun @zhiics @icemelon9 @jwfromm @jroesch

anijain2305 · 2019-11-25T04:56:20Z

include/tvm/relay/attrs/image.h

+                  "bilinear - Bilinear Interpolation");
+    TVM_ATTR_FIELD(extrapolation_value).set_default(0.0)
+        .describe("Specify value for extrapolation.");
+    TVM_ATTR_FIELD(out_dtype)


Why is out_dtype needed? Shouldn't the out dtype = input dtype?

It's actually useful to be able to specify it. Most images you'd be applying this to have datatype uint8. Bilinear or Bicubic resizing will by default output floats due to their interpolation but there may be cases where you'd like to stay as an integer type. Similarly, nearest nearest neighbor wont change the output type to float since it has no interpolation. Being able to specify a fixed output type improves the predictability of the function.

Makes sense.

anijain2305 · 2019-11-25T04:57:50Z

include/tvm/relay/attrs/image.h

+  TVM_DECLARE_ATTRS(CropAndResizeAttrs, "relay.attrs.CropAndResizeAttrs") {
+    TVM_ATTR_FIELD(crop_size).set_default(NullValue<Array<IndexExpr> >())
+        .describe("Target Size.");
+    TVM_ATTR_FIELD(layout).set_default("NCHW")


Does it make sense to keep layout out?

Resize requires knowing which axes are the spatial dimensions, so having the layout as an attribute is necessary.

Makes sense.

topi/python/topi/image/resize.py

jwfromm

This is definitely a good addition, crop_and_resize is a common op and having direct support for it will be useful. My only complaint with this implementation is the heavy duplication between resize and crop_and_resize, which I think can be dramatically reduced with a little refactoring. I recommend pulling out the kernels for nearest_neighbor, bilinear, and bicubic sampling and making them general enough to fit both types of resize.

python/tvm/relay/op/image/image.py

topi/python/topi/image/resize.py

anijain2305 · 2019-11-25T19:18:29Z

topi/python/topi/image/resize.py

+        n, c, y, x, cc = _get_indices(*indices)
+        box_idx = box_indices(n)
+
+        y1, x1 = boxes(n, 0), boxes(n, 1)
+        y2, x2 = boxes(n, 2), boxes(n, 3)
+
+        in_h = (image_height - 1) * (y2 - y1)
+        in_w = (image_width - 1) * (x2 - x1)
+        h_scale = tvm.div(in_h, target_h - 1)
+        w_scale = tvm.div(in_w, target_w - 1)


Agree with @jwfromm This seeems to be repeated.

It is sligthtly different from crop, for example, h_scale is calculated based on boxes while crop has no boxes, etc. I have moved these kernels out of the op and generalized them to work for both resize and crop_and_resize.

topi/python/topi/image/resize.py

yongwww · 2019-11-26T22:56:30Z

have addressed the comments, please take a look if you have some cycles @jwfromm @anijain2305 @kevinthesun

jwfromm · 2019-11-27T06:13:10Z

Looking at this again, I was wondering if there's any reason we can't implement a separate topi.crop function then have topi.crop_and_resize simply first call topi.crop then call topi.resize. That seems like it would be by far the cleanest implementation.

yongwww · 2019-11-27T08:09:54Z

Looking at this again, I was wondering if there's any reason we can't implement a separate topi.crop function then have topi.crop_and_resize simply first call topi.crop then call topi.resize. That seems like it would be by far the cleanest implementation.

I guess crop here is the same as strided_slice? I was thinking this for a while, besides the reasons in the pr description above, some of the other reasons are as following:

Having a single op crop_and_resize, we don't crop the input and then resize sequentially. crop_and_resize directly resizes the input with target size. However, if crop first, then resize, it has to iterate through input images twice, which would introduce some overhead, I guess it is one of the important reasons why TF has crop_and_resize.
"Crop and then Resize" is not able to handle interpolation with extrapolation_value > 0 (sorry TF has this), the extrapolation_value is related to boxes, original image height and width, that means we have to change resize to take some extra inputs like boxes and original (NOT the cropped) image height and width, seems it is not simple/clean.

jwfromm · 2019-11-28T00:37:04Z

Ok, those explanations make sense. In that case I like the updates you made. LGTM once you get the tests passing.

kevinthesun · 2019-12-01T00:19:48Z

Overall lgtm. @yongwww Could you take a look at the ci?

yongwww · 2020-01-09T23:20:20Z

@kevinthesun @zhiics @jwfromm @anijain2305 fixed comments and ci. Please take another look

zhiics

LGTM

Other folks please take another look as well.

jwfromm

LGTM! Nice job getting this all working. I think it turned out great.

zhiics · 2020-01-11T03:53:19Z

Thanks @yongwww @jwfromm @kevinthesun @anijain2305

* [TOPI][RELAY][OP] add op crop_and_resize * fix pylint * incorporate comments * fix ci

anijain2305 reviewed Nov 25, 2019

View reviewed changes

jwfromm reviewed Nov 25, 2019

View reviewed changes

kevinthesun reviewed Nov 25, 2019

View reviewed changes

python/tvm/relay/op/image/image.py Outdated Show resolved Hide resolved

anijain2305 reviewed Nov 25, 2019

View reviewed changes

yongwww force-pushed the crop_resize branch from f68787e to 0f86f61 Compare November 27, 2019 01:54

yongwww force-pushed the crop_resize branch from 0f86f61 to a9542dd Compare November 27, 2019 08:23

yongwww force-pushed the crop_resize branch 2 times, most recently from 17a4479 to 02b36c1 Compare December 8, 2019 19:39

yongwww force-pushed the crop_resize branch 4 times, most recently from 19537ba to 4d894b0 Compare January 9, 2020 21:38

yongwww added 4 commits January 9, 2020 15:32

[TOPI][RELAY][OP] add op crop_and_resize

34ca555

fix pylint

8874f8c

incorporate comments

6e2c976

fix ci

e36cfac

yongwww force-pushed the crop_resize branch from 4d894b0 to e36cfac Compare January 9, 2020 23:41

zhiics approved these changes Jan 10, 2020

View reviewed changes

jwfromm approved these changes Jan 11, 2020

View reviewed changes

zhiics merged commit 56416ed into apache:master Jan 11, 2020

zhiics added the status: accepted label Jan 11, 2020

alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 26, 2020

[TOPI][RELAY][OP] add op crop_and_resize (apache#4417)

71c1106

* [TOPI][RELAY][OP] add op crop_and_resize * fix pylint * incorporate comments * fix ci

alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 28, 2020

[TOPI][RELAY][OP] add op crop_and_resize (apache#4417)

4675a76

* [TOPI][RELAY][OP] add op crop_and_resize * fix pylint * incorporate comments * fix ci

zhiics pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2020

[TOPI][RELAY][OP] add op crop_and_resize (apache#4417)

cc77775

* [TOPI][RELAY][OP] add op crop_and_resize * fix pylint * incorporate comments * fix ci

ZihengJiang mentioned this pull request Sep 17, 2020

TVM v0.7 Release Note Candidate #6486

Closed

yongwww deleted the crop_resize branch December 8, 2020 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TOPI][RELAY][OP] add op crop_and_resize #4417

[TOPI][RELAY][OP] add op crop_and_resize #4417

yongwww commented Nov 24, 2019 •

edited

Loading

anijain2305 Nov 25, 2019

jwfromm Nov 25, 2019

anijain2305 Nov 25, 2019

anijain2305 Nov 25, 2019

jwfromm Nov 25, 2019

anijain2305 Nov 25, 2019

jwfromm left a comment •

edited

Loading

anijain2305 Nov 25, 2019

yongwww Nov 26, 2019

yongwww commented Nov 26, 2019

jwfromm commented Nov 27, 2019

yongwww commented Nov 27, 2019

jwfromm commented Nov 28, 2019

kevinthesun commented Dec 1, 2019

yongwww commented Jan 9, 2020

zhiics left a comment

jwfromm left a comment

zhiics commented Jan 11, 2020

[TOPI][RELAY][OP] add op crop_and_resize #4417

[TOPI][RELAY][OP] add op crop_and_resize #4417

Conversation

yongwww commented Nov 24, 2019 • edited Loading

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

jwfromm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongwww commented Nov 26, 2019

jwfromm commented Nov 27, 2019

yongwww commented Nov 27, 2019

jwfromm commented Nov 28, 2019

kevinthesun commented Dec 1, 2019

yongwww commented Jan 9, 2020

zhiics left a comment

Choose a reason for hiding this comment

jwfromm left a comment

Choose a reason for hiding this comment

zhiics commented Jan 11, 2020

yongwww commented Nov 24, 2019 •

edited

Loading

jwfromm left a comment •

edited

Loading