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

[MXNET-378] Adding depth_to_space and space_to_depth operator(Updated) #11587

Merged
merged 7 commits into from
Jul 26, 2018
Merged

[MXNET-378] Adding depth_to_space and space_to_depth operator(Updated) #11587

merged 7 commits into from
Jul 26, 2018

Conversation

access2rohit
Copy link
Contributor

@access2rohit access2rohit commented Jul 6, 2018 •
edited
Loading

Description

Added new operators "depth_to_space" and "space_to_depth" to mxnet

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://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

  • Added new operators "depth_to_space" and "space_to_depth"

Comments

Ran Tests on CPU and GPU for both depth_to_space and space_to_depth operator 10000 times

@access2rohit
Copy link
Contributor Author

@haojin2

@access2rohit access2rohit requested a review from szha as a code owner July 6, 2018 20:58
@access2rohit access2rohit changed the title [MXNET-378] Adding depth_to_space and space_to_depth operator [MXNET-378] Adding depth_to_space and space_to_depth operator(Updated) Jul 6, 2018
@haojin2
Copy link
Contributor

haojin2 commented Jul 12, 2018

@piiswrong @reminisce @anirudh2290 @eric-haibin-lin @rahul003 Please help with review when you have time, thanks!

eric-haibin-lin
eric-haibin-lin reviewed Jul 13, 2018
View reviewed changes
src/operator/tensor/matrix_op.cc Outdated Show resolved Hide resolved
src/operator/tensor/matrix_op.cc Outdated Show resolved Hide resolved
src/operator/tensor/matrix_op.cu
@@ -211,5 +211,11 @@ NNVM_REGISTER_OP(squeeze)
NNVM_REGISTER_OP(_backward_squeeze)
.set_attr<FCompute>("FCompute<gpu>", UnaryOp::IdentityCompute<gpu>);

NNVM_REGISTER_OP(depth_to_space)
Copy link
Member

Choose a reason for hiding this comment

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

Please also update https://github.com/apache/incubator-mxnet/blob/master/docs/api/python/ndarray/ndarray.md and docs/api/python/symbol/symbol.md when adding new ops

anirudh2290
anirudh2290 suggested changes Jul 14, 2018
View reviewed changes
Copy link
Member

@anirudh2290 anirudh2290 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Good work.

src/operator/tensor/matrix_op.cc Outdated Show resolved Hide resolved
src/operator/tensor/matrix_op.cc Outdated Show resolved Hide resolved
src/operator/tensor/matrix_op.cc Outdated Show resolved Hide resolved
src/operator/tensor/matrix_op.cc Outdated Show resolved Hide resolved
tests/python/unittest/test_operator.py Outdated Show resolved Hide resolved
tests/python/unittest/test_operator.py Outdated Show resolved Hide resolved
src/operator/tensor/matrix_op-inl.h Show resolved Hide resolved
src/operator/tensor/matrix_op-inl.h Show resolved Hide resolved
src/operator/tensor/matrix_op-inl.h Outdated Show resolved Hide resolved
src/operator/tensor/matrix_op-inl.h Outdated Show resolved Hide resolved
@access2rohit access2rohit force-pushed the master branch 2 times, most recently from ce29f04 to 0187ca1 Compare July 23, 2018 23:47
@access2rohit
Copy link
Contributor Author

@rahul003 @reminisce @anirudh2290 @eric-haibin-lin @azai91

haojin2
haojin2 reviewed Jul 24, 2018
View reviewed changes
Copy link
Contributor

@haojin2 haojin2 left a comment

Choose a reason for hiding this comment

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

In general LGTM but please introduce the randomized tests.

@rahul003
Copy link
Member

Agree with @haojin2 would be good to have some random tests

eric-haibin-lin
eric-haibin-lin previously requested changes Jul 25, 2018
View reviewed changes
Copy link
Member

@eric-haibin-lin eric-haibin-lin left a comment

Choose a reason for hiding this comment

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

The link doesn't look right.
http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-11587/14/api/python/ndarray/ndarray.html#array-rearrange-elements

The document is not very readable and the format seems off.
http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-11587/14/api/python/ndarray/ndarray.html#mxnet.ndarray.depth_to_space

@eric-haibin-lin eric-haibin-lin dismissed their stale review July 26, 2018 20:44

looks good now

@eric-haibin-lin eric-haibin-lin merged commit c13ce5c into apache:master Jul 26, 2018
XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
@access2rohit @eric-haibin-lin
[MXNET-378] Adding depth_to_space and space_to_depth operator(Updated) (
69fe157 
apache#11587)

* [MXNET-378] Adding depth_to_space and space_to_depth operator

* fixed lint and windows CPU errors

* compliance with C++ style guiide and address shortcomings in unittests

* fixed documentation and nitpicky suggestions

* added operator references in API docs and removed inplace optimization support

* Added references in symbol.md and ndarray.md. Improved test cases and added block_size check

* Fixing bugs in documentation. Tests now include tensors of random shapes.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@haojin2 haojin2 haojin2 left review comments

@anirudh2290 anirudh2290 anirudh2290 approved these changes

@rahul003 rahul003 rahul003 approved these changes

@eric-haibin-lin eric-haibin-lin eric-haibin-lin left review comments

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@access2rohit @haojin2 @rahul003 @anirudh2290 @eric-haibin-lin