Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Image normalize operator - GPU support, 3D/4D inputs #13802

Merged

Conversation

sandeep-krishnamurthy
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy commented Jan 8, 2019

Description

  1. Make normalize as operator. No contribution to gradient. Add backward function to normalize operator.
  2. Support CPU and GPU. This PR adds GPU supports and re-organizes previously supported CPU.
  3. 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.
  4. Parallelization with OMP
  5. 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

@apeforest @stu1130 @zhreshold @nswamy - For review

src/operator/image/normalize_op-inl.h Outdated Show resolved Hide resolved
src/operator/image/normalize_op-inl.h Outdated Show resolved Hide resolved
src/operator/image/normalize_op.cc Outdated Show resolved Hide resolved
[](const NodeAttrs& attrs) {
return std::vector<std::pair<int, int> >{{0, 0}};
})
.set_attr<nnvm::FGradient>("FGradient", ElemwiseGradUseNone{ "_copy" })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some high-level issues with this PR:

Make normalize as operator. No contribution to gradient.

  1. 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.
  2. 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
szha previously requested changes Jan 17, 2019
Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to request changes for the above.

@sandeep-krishnamurthy
Copy link
Contributor Author

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

@sandeep-krishnamurthy
Copy link
Contributor Author

@szha

  1. 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.
  2. 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
Copy link
Member

szha commented Jan 18, 2019

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
Copy link
Contributor Author

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
Copy link
Member

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

@sandeep-krishnamurthy
Copy link
Contributor Author

@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 Outdated Show resolved Hide resolved
src/operator/image/image_random-inl.h Outdated Show resolved Hide resolved
src/operator/image/image_random-inl.h Outdated Show resolved Hide resolved
src/operator/image/image_random-inl.h Outdated Show resolved Hide resolved
src/operator/image/image_random.cc Outdated Show resolved Hide resolved
@sandeep-krishnamurthy
Copy link
Contributor Author

@zhreshold - Thanks! I addressed your review comments.

src/operator/image/image_random-inl.h Outdated Show resolved Hide resolved
src/operator/image/image_random.cc Show resolved Hide resolved
@szha szha dismissed their stale review January 28, 2019 22:11

concerns addressed.

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit 3a1a80a into apache:master Jan 29, 2019
sandeep-krishnamurthy added a commit to sandeep-krishnamurthy/incubator-mxnet that referenced this pull request Feb 1, 2019
jlcontreras pushed a commit to jlcontreras/incubator-mxnet that referenced this pull request Feb 14, 2019
* 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
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
* 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
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Gluon Operator pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants