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

Implementation for equivalence of tf.moments #14842

Merged
merged 1 commit into from
May 19, 2019
Merged

Conversation

haojin2
Copy link
Contributor

@haojin2 haojin2 commented Apr 30, 2019

Description

https://www.tensorflow.org/api_docs/python/tf/nn/moments

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

  • Forward & Backward
  • Unit Test

Comments

Related to #11283, pre-requisite of group norm implementation.
Flakiness Check:

MXNET_TEST_COUNT=10000 nosetests tests/python/unittest/test_operator.py:test_moments
[INFO] Setting module np/mx/python random seeds, use MXNET_MODULE_SEED=862626152 to reproduce.
.
----------------------------------------------------------------------
Ran 1 test in 1380.770s

OK
MXNET_TEST_COUNT=10000 nosetests tests/python/gpu/test_operator_gpu.py:test_moments
[INFO] Setting module np/mx/python random seeds, use MXNET_MODULE_SEED=688103580 to reproduce.
.
----------------------------------------------------------------------
Ran 1 test in 2116.433s

OK

@haojin2 haojin2 self-assigned this Apr 30, 2019
@haojin2 haojin2 force-pushed the moments branch 2 times, most recently from f228fef to c32450c Compare April 30, 2019 22:44
@haojin2 haojin2 changed the title [WIP] Implementation for equivalence of tf.moments Implementation for equivalence of tf.moments Apr 30, 2019
@haojin2
Copy link
Contributor Author

haojin2 commented May 1, 2019

@eric-haibin-lin @reminisce @szha ping for review.

@wkcn
Copy link
Member

wkcn commented May 2, 2019

Great! Is it convenient to support the tensor with arbitrary dimension? Thank you!

@haojin2
Copy link
Contributor Author

haojin2 commented May 2, 2019

One thing is that usually tensors do not have >5 dims. The kernel can actually handle any # of dims, but we need to modify the mxnet::TShape/Tuple class to enable GPUs to read from mxnet::TShape/Tuple.

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.

Would you mind sharing some common use cases for this operator? Thanks!

@haojin2
Copy link
Contributor Author

haojin2 commented May 6, 2019

@eric-haibin-lin The op gives variance of the input on arbitrary axis/axes, so it's technically the np.var operator's equivalence with an extra mean output. The impl of this op could also become the common helper function for various versions of normalizations.
Plus: My current work on group norm is depending on the impl of this op or I'll have to write the same impl twice within the operator.

@haojin2
Copy link
Contributor Author

haojin2 commented May 8, 2019

@eric-haibin-lin @hetong007 Is this good for merge?

@hetong007
Copy link
Contributor

LGTM

@haojin2
Copy link
Contributor Author

haojin2 commented May 10, 2019

@eric-haibin-lin @szha Can any of you take a final look and help with merging this op? Thanks!

@haojin2
Copy link
Contributor Author

haojin2 commented May 16, 2019

@eric-haibin-lin @szha Ping for one more review.

@szha szha merged commit b377130 into apache:master May 19, 2019
@haojin2 haojin2 deleted the moments branch May 20, 2019 07:39
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants