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

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

Merged
merged 10 commits into from
Feb 4, 2019
Merged

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

merged 10 commits into from
Feb 4, 2019

Conversation

sandeep-krishnamurthy
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy commented Jan 10, 2019 •
edited
Loading

Description

  1. Make to_tensor as operator. No contribution to gradient.Add description, InPlaceOption and other parameters during operator registration.
  2. Support CPU and GPU. This PR adds GPU supports and re-organizes previously supported CPU.
  3. to_tensor 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.
Related PR for normalize transformation - #13802

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)
  • 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 to_tensor to accept 3D or 4D input
  • CPU / GPU support
  • No gradient contribution
  • Reorganize like any other operators

Comments

@apeforest @stu1130 @zhreshold @nswamy - For review

stu1130
stu1130 reviewed Jan 11, 2019
View reviewed changes
Copy link
Contributor

@stu1130 stu1130 left a comment

Choose a reason for hiding this comment

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

should we unify the unit test file name of cpu and gpu

src/operator/image/totensor_op-inl.h Outdated Show resolved Hide resolved
szha
szha previously requested changes Jan 17, 2019
View reviewed changes
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 to_tensor as operator. No contribution to gradient.

  1. based on the description, this PR seems to assume that to_tensor is not yet an operator. but it is!. to_tensor 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 edits in the existing files.

@sandeep-krishnamurthy
Copy link
Contributor Author

@stu1130 - Thanks, I have addressed your comments. Can you please take a look?
@zhreshold - Can you please help review this PR?

@sandeep-krishnamurthy
Copy link
Contributor Author

@szha
As commented in other PR - #13802

  1. I did not mean it is not an operator. I meant in this PR we reorganize, cpu/gpu support, 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.

@zhreshold
Copy link
Member

@sandeep-krishnamurthy Can you keep the ops in the original files for sake of easier review? For now I get lost what has actually been modifed in addtion to the original implementation.

If you find the name of the file not appropriate, you can file a separate PR to move it and notify me that the new PR is purely to re-org code.

@zhreshold zhreshold mentioned this pull request Jan 24, 2019
Merged
4 tasks
@sandeep-krishnamurthy
Copy link
Contributor Author

@sandeep-krishnamurthy Can you keep the ops in the original files for sake of easier review? For now I get lost what has actually been modifed in addtion to the original implementation.

If you find the name of the file not appropriate, you can file a separate PR to move it and notify me that the new PR is purely to re-org code.

Yes. I was working on moving it back per @szha suggestion.

@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!

@sandeep-krishnamurthy
Copy link
Contributor Author

@szha - I have addressed your comments. Can you please take a look at this PR? Thanks.

@szha szha dismissed their stale review February 1, 2019 00:49

addressed concerns

stu1130
stu1130 approved these changes Feb 4, 2019
View reviewed changes
Copy link
Contributor

@stu1130 stu1130 left a comment

Choose a reason for hiding this comment

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

LGTM

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit f86f21e into apache:master Feb 4, 2019
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
@sandeep-krishnamurthy @stephenrawls
Image ToTensor operator - GPU support, 3D/4D inputs (apache#13837)
45806e7 
* Add CPU implementation of ToTensor

* Add tests for cpu

* Add gpu implementation and tests

* Fix lint issues

* Cleanup includes

* Move back changes to original image operators files

* Add 4D example

* resolve merge conflicts

* Fix failing tests

* parallelize on channel in kernel launch
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@sandeep-krishnamurthy @vdantu
Image ToTensor operator - GPU support, 3D/4D inputs (apache#13837)
d9f36a5 
* Add CPU implementation of ToTensor

* Add tests for cpu

* Add gpu implementation and tests

* Fix lint issues

* Cleanup includes

* Move back changes to original image operators files

* Add 4D example

* resolve merge conflicts

* Fix failing tests

* parallelize on channel in kernel launch
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@sandeep-krishnamurthy
Image ToTensor operator - GPU support, 3D/4D inputs (apache#13837)
329efbb 
* Add CPU implementation of ToTensor

* Add tests for cpu

* Add gpu implementation and tests

* Fix lint issues

* Cleanup includes

* Move back changes to original image operators files

* Add 4D example

* resolve merge conflicts

* Fix failing tests

* parallelize on channel in kernel launch
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stu1130 stu1130 stu1130 approved these changes

@zhreshold zhreshold zhreshold approved these changes

@szha szha szha left review comments

Assignees
No one assigned
Labels
Gluon Operator pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sandeep-krishnamurthy @szha @zhreshold @stu1130