Image normalize operator - GPU support, 3D/4D inputs by sandeep-krishnamurthy · Pull Request #13802 · apache/mxnet

sandeep-krishnamurthy · 2019-01-08T21:23:41Z

Description

~~Make normalize as operator. No contribution to gradient.~~ Add backward function to normalize operator.
Support CPU and GPU. This PR adds GPU supports and re-organizes previously supported CPU.
Normalize transformation operator can take 3D (c, h, w) or 4D (n, c, h, w) at once. So that a batch of input can be passed in the transformation.
Parallelization with OMP
This is backward compatible change. No existing functionality will be affected.

Background

We can enable users to fuse transformation operators with network graph and export it. This end to end network with transforms + network will greatly simplify inference deployment.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

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.
Check the API doc at http://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

Make Normalize to accept 3D or 4D input
CPU / GPU support
No gradient contribution
Reorganize like any other operators

Comments

A related PR was raised here Make to_tensor and normalize to accept 3D or 4D tensor inputs #13614 reviewed by @apeforest and @stu1130 - I have addressed those comments. I am closing other PR as it was mixing multiple transforms and functionality.

@apeforest @stu1130 @zhreshold @nswamy - For review

src/operator/image/normalize_op-inl.h

src/operator/image/normalize_op.cc

zhreshold · 2019-01-11T19:13:48Z

src/operator/image/normalize_op.cc

+  [](const NodeAttrs& attrs) {
+    return std::vector<std::pair<int, int> >{{0, 0}};
+  })
+.set_attr<nnvm::FGradient>("FGradient", ElemwiseGradUseNone{ "_copy" })


copy gradient is not correct IMO, even though this OP rarely pass gradient back, we still need to take care of this

Thanks for flagging this. Yes you are right, it is should out_grad * 1/std_dev for each channel. Fixed and also added tests for symbolic forward/backward and gradient checks.

szha

Some high-level issues with this PR:

Make normalize as operator. No contribution to gradient.

based on the description, this PR seems to assume that normalize is not yet an operator. but it is!. normalize is an operator in the image namespace.
the unnecessary file movement should be reverted so that edit history of the code are best preserved. suggest to make changes in the existing files.

szha

Meant to request changes for the above.

sandeep-krishnamurthy · 2019-01-17T22:24:56Z

@stu1130 @zhreshold - Thanks for your review. Addressed your comments. Can you please take a look? Thanks.

sandeep-krishnamurthy · 2019-01-17T22:34:39Z

@szha

I did not mean it is not an operator. I meant in this PR we reorganize, cpu/gpu support, backward pass and more such properties of an operator are added. Reworded the description.
IMO, the reorganizing and creation of new files for this operator is in the right direction and unifies with other operator pattern. Having all these implementation in image_random-inl.h creates more confusion and out of order compared to other operators.

szha · 2019-01-18T00:22:40Z

From first glance there doesn't seem to be that many changes to the extent that it would cause confusion. Let's at least give it a try.

sandeep-krishnamurthy · 2019-01-18T06:03:22Z

From first glance there doesn't seem to be that many changes to the extent that it would cause confusion. Let's at least give it a try.

I can surely do that, but, I don't understand/convinced the motivation for doing that. Can you please help me understand? If code change history is the only reason, this commit, PR will surely help track and provide the history. For doing this way, like I said earlier, this is just the way every other operator is done in MXNet, and more over normalize is not a random augmentation operator to be part of image-random-inl.h. Also, there will be more functionalities for example supporting "list input" and so on.

zhreshold · 2019-01-24T21:13:54Z

Similar to #13837 (comment)
can you keep the original file for easier review? @sandeep-krishnamurthy

sandeep-krishnamurthy · 2019-01-24T22:38:48Z

@zhreshold , @szha - I moved back all changes to same files as per suggestion. Can you please take a look at this PR? Thanks!

src/operator/image/image_random-inl.h

src/operator/image/image_random.cc

sandeep-krishnamurthy · 2019-01-25T21:32:43Z

@zhreshold - Thanks! I addressed your review comments.

src/operator/image/image_random-inl.h

src/operator/image/image_random.cc

… cc and cu

concerns addressed.

…13802)" This reverts commit 3a1a80a.

* CPU version of normalize operator is working and unit test added * Add GPU implementation and tests * Working GPU normalize transforms * Add default values, fix imports, fix documentation * Add backward implmentation for image normalize * Add tests for backward pass * Move back operators to its original files * Add review comments * Add 4D example * Make infer type generic * Fix inline function build error * make functions as inline to avoid multiple definition conflict across cc and cu * Fix build errors * Fix failing GPU tests

sandeep-krishnamurthy requested a review from szha as a code owner January 8, 2019 21:23

sandeep-krishnamurthy mentioned this pull request Jan 8, 2019

Make to_tensor and normalize to accept 3D or 4D tensor inputs #13614

Closed

4 tasks

sandeep-krishnamurthy added Operator Gluon pr-awaiting-review PR is waiting for code review labels Jan 8, 2019

sandeep-krishnamurthy mentioned this pull request Jan 10, 2019

Image ToTensor operator - GPU support, 3D/4D inputs #13837

Merged

4 tasks

stu1130 reviewed Jan 11, 2019

View reviewed changes

src/operator/image/normalize_op-inl.h Outdated Show resolved Hide resolved

zhreshold suggested changes Jan 11, 2019

View reviewed changes

sandeep-krishnamurthy force-pushed the normalize_trans branch from 0b5dd72 to 7fdced9 Compare January 14, 2019 22:13

szha reviewed Jan 17, 2019

View reviewed changes

szha previously requested changes Jan 17, 2019

View reviewed changes

sandeep-krishnamurthy force-pushed the normalize_trans branch from 7011380 to 149eb37 Compare January 24, 2019 22:35

zhreshold suggested changes Jan 25, 2019

View reviewed changes

zhreshold suggested changes Jan 28, 2019

View reviewed changes

src/operator/image/image_random-inl.h Outdated Show resolved Hide resolved

src/operator/image/image_random.cc Show resolved Hide resolved

zhreshold approved these changes Jan 28, 2019

View reviewed changes

sandeepkrishnamurthy-dev added 8 commits January 28, 2019 13:33

CPU version of normalize operator is working and unit test added

6034b42

Add GPU implementation and tests

e38af2d

Working GPU normalize transforms

adcb6f7

Add default values, fix imports, fix documentation

d49308c

Add backward implmentation for image normalize

078de8b

Add tests for backward pass

c51ecb3

Move back operators to its original files

be92608

Add review comments

2fc6076

sandeepkrishnamurthy-dev added 3 commits January 28, 2019 13:33

Add 4D example

5ccb8b9

Make infer type generic

6068fd2

Fix inline function build error

83c21a9

sandeep-krishnamurthy force-pushed the normalize_trans branch from a803806 to 83c21a9 Compare January 28, 2019 21:33

make functions as inline to avoid multiple definition conflict across…

177701b

… cc and cu

sandeepkrishnamurthy-dev added 2 commits January 28, 2019 14:20

Fix build errors

974bf46

Fix failing GPU tests

3e1437e

sandeep-krishnamurthy merged commit 3a1a80a into apache:master Jan 29, 2019

sandeep-krishnamurthy pushed a commit to sandeep-krishnamurthy/incubator-mxnet that referenced this pull request Feb 1, 2019

Revert "Image normalize operator - GPU support, 3D/4D inputs (apache#…

6d76f8b

…13802)" This reverts commit 3a1a80a.

This was referenced Feb 2, 2019

Revert "Image normalize operator - GPU support, 3D/4D inputs (#13802)" #14054

Closed

Fix performance regression in normalize operator #14055

Merged

Conversation

sandeep-krishnamurthy commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Background

Checklist

Essentials

Changes

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhreshold Jan 11, 2019

Choose a reason for hiding this comment

Uh oh!

sandeep-krishnamurthy Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

szha left a comment

Choose a reason for hiding this comment

Uh oh!

szha left a comment

Choose a reason for hiding this comment

Uh oh!

sandeep-krishnamurthy commented Jan 17, 2019

Uh oh!

sandeep-krishnamurthy commented Jan 17, 2019

Uh oh!

szha commented Jan 18, 2019

Uh oh!

sandeep-krishnamurthy commented Jan 18, 2019

Uh oh!

zhreshold commented Jan 24, 2019

Uh oh!

sandeep-krishnamurthy commented Jan 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandeep-krishnamurthy commented Jan 25, 2019

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sandeep-krishnamurthy commented Jan 8, 2019 •

edited

Loading