Add Gluon Transformer Crop #14259

stu1130 · 2019-02-26T23:23:50Z

Description

Add crop operator and gluon transformer front
support batch input
modify the fixed_crop with slice

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http:https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Comments

@zhreshold

anirudhacharya · 2019-02-27T20:58:20Z

@mxnet-label-bot add [pr-awaiting-review]

@stu1130 is this trying to achieve the same functionality as this PR - #13679

have the comments in the previous PR( 1 and 2) been addressed here? could you please elaborate on how the comments have been addressed in this new PR?

stu1130 · 2019-02-27T21:32:52Z

@anirudhacharya yes it is
For the first one, the resize functionality is moved out of Crop operator so no .value() is needed. And I added the usage and doc for it.
For the second one, if we want to make fixed_crop take advantage of my operator ,it would be one line change from using the nd.crop to nd.image.crop. Nothing sinificant change. so I kept the original one.

azai91 · 2019-02-27T21:44:42Z

what is the behavior if the crop size is greater than the input image for either dimension?

stu1130 · 2019-02-27T21:50:53Z

@azai91 throw MXNet exception
https://github.com/apache/incubator-mxnet/blob/54e746b12018417df2c37e32a73bb0dee87df492/src/operator/image/crop-inl.h#L97-L100

azai91 · 2019-02-27T21:54:01Z

got it, thanks

zhreshold · 2019-02-27T22:19:03Z

python/mxnet/gluon/data/vision/transforms.py

@@ -228,6 +228,54 @@ def forward(self, x):
 return image.random_size_crop(x, *self._args)[0]


+class Crop(HybridBlock):


there is an deprecated mx.sym.Crop op already, using sym.image.Crop may introduce some confusion again, can you propose a new name?

For image transformation, since resize is supported, I guess CropResize is better?

Do you intend to expand the implementation to add more parameters like x1, y1 ? If so then it makes sense to keep them x0 and y0. Else I think you should change them to x and y

thanks @access2rohit I assume now I only need one x and y so I'll go with x and y

zhreshold · 2019-02-27T22:19:52Z

python/mxnet/gluon/data/vision/transforms.py

+ Makes a crop of the original image then optionally resize it to the specified size.
+ Parameters
+ ----------
+ x0 : int


imo, x is better than x0 since there is nox1

zhreshold · 2019-02-27T22:20:00Z

python/mxnet/gluon/data/vision/transforms.py

+ ----------
+ x0 : int
+ Left boundary of the cropping area
+ y0 : int


zhreshold · 2019-02-27T22:22:58Z

src/operator/image/crop.cc

+.set_attr<nnvm::FInferShape>("FInferShape", CropShape)
+.set_attr<nnvm::FInferType>("FInferType", ElemwiseType<1, 1>)
+.set_attr<FCompute>("FCompute<cpu>", Crop)
+.set_attr<nnvm::FGradient>("FGradient", ElemwiseGradUseNone{ "_copy" })


copy gradient does not apply to crop op.

vandanavk · 2019-03-04T22:24:53Z

python/mxnet/gluon/data/vision/transforms.py

+
+ def hybrid_forward(self, F, x):
+ out = F.image.crop(x, self._x0, self._y0, self._width, self._height)
+ if self._size is not None:


if self._size

vandanavk · 2019-03-04T22:26:50Z

src/operator/image/crop-inl.h

+ DMLC_DECLARE_FIELD(width)
+ .describe("Width of the cropping area.");
+ DMLC_DECLARE_FIELD(height)
+ .describe("Top boundary of the cropping area");


Height of the cropping area?

stu1130 · 2019-03-15T02:00:45Z

@zhreshold ready for 2nd round review. Thanks

zhreshold

Thanks for addressing previous comments, I see no issue, but can you add unittest for backward gradient check?

stu1130 · 2019-04-02T19:58:29Z

@zhreshold I added the unit test for backward gradient check

sandeep-krishnamurthy

Thanks. Few minor comments.

python/mxnet/gluon/data/vision/transforms.py

src/operator/image/crop-inl.h

sandeep-krishnamurthy

LGTM. Thanks

python/mxnet/gluon/data/vision/transforms.py

* implement crop * add crop operator * fix for linter * add. backword and refactor the code * fix error namespace * fix the website build failure * start adding the unit test of backword * add unit test for backward * address the comment * add missing statement * fix the website error * fix the website building * add missing doc

stu1130 requested a review from szha as a code owner February 26, 2019 23:23

marcoabreu added the pr-awaiting-review PR is waiting for code review label Feb 27, 2019

zhreshold suggested changes Feb 27, 2019

View reviewed changes

vandanavk reviewed Mar 4, 2019

View reviewed changes

stu1130 force-pushed the gluon_image_crop branch from 6e101b7 to 0dd7407 Compare March 14, 2019 21:13

zhreshold suggested changes Mar 18, 2019

View reviewed changes

stu1130 force-pushed the gluon_image_crop branch from faffe1b to 40410d0 Compare April 2, 2019 17:59

zhreshold approved these changes Apr 2, 2019

View reviewed changes

stu1130 force-pushed the gluon_image_crop branch from 40410d0 to 3f6583d Compare April 2, 2019 23:23

sandeep-krishnamurthy suggested changes Apr 3, 2019

View reviewed changes

sandeep-krishnamurthy approved these changes Apr 3, 2019

View reviewed changes

sandeep-krishnamurthy added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Apr 3, 2019

stu1130 added 10 commits April 3, 2019 12:12

implement crop

47d27af

add crop operator

8c1254b

fix for linter

460694a

add. backword and refactor the code

9ceda61

fix error namespace

c015d47

fix the website build failure

d0da9a8

start adding the unit test of backword

6dc6bcc

add unit test for backward

ac738c0

address the comment

11dbda7

add missing statement

e738869

stu1130 added 2 commits April 3, 2019 12:12

fix the website error

7aabda6

fix the website building

352dede

sandeep-krishnamurthy reviewed Apr 3, 2019

View reviewed changes

python/mxnet/gluon/data/vision/transforms.py Show resolved Hide resolved

add missing doc

2926495

stu1130 force-pushed the gluon_image_crop branch from d3d92ce to 2926495 Compare April 3, 2019 21:13

sandeep-krishnamurthy merged commit 5241c1b into apache:master Apr 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Gluon Transformer Crop #14259

Add Gluon Transformer Crop #14259

stu1130 commented Feb 26, 2019

anirudhacharya commented Feb 27, 2019

stu1130 commented Feb 27, 2019 •

edited

Loading

azai91 commented Feb 27, 2019

stu1130 commented Feb 27, 2019 •

edited

Loading

azai91 commented Feb 27, 2019

zhreshold Feb 27, 2019

access2rohit Feb 28, 2019

stu1130 Feb 28, 2019

zhreshold Feb 27, 2019

zhreshold Feb 27, 2019

zhreshold Feb 27, 2019

vandanavk Mar 4, 2019

vandanavk Mar 4, 2019

stu1130 commented Mar 15, 2019

zhreshold left a comment

stu1130 commented Apr 2, 2019

sandeep-krishnamurthy left a comment

sandeep-krishnamurthy left a comment

		@@ -228,6 +228,54 @@ def forward(self, x):
		return image.random_size_crop(x, *self._args)[0]


		class Crop(HybridBlock):

Add Gluon Transformer Crop #14259

Add Gluon Transformer Crop #14259

Conversation

stu1130 commented Feb 26, 2019

Description

Checklist

Essentials

Changes

Comments

anirudhacharya commented Feb 27, 2019

stu1130 commented Feb 27, 2019 • edited Loading

azai91 commented Feb 27, 2019

stu1130 commented Feb 27, 2019 • edited Loading

azai91 commented Feb 27, 2019

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

stu1130 commented Mar 15, 2019

zhreshold left a comment

Choose a reason for hiding this comment

stu1130 commented Apr 2, 2019

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

stu1130 commented Feb 27, 2019 •

edited

Loading

stu1130 commented Feb 27, 2019 •

edited

Loading