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

Support kAddTo for softmax backward#11836

Merged
anirudh2290 merged 1 commit intoapache:masterfrom
haojin2:softmax_addto
Aug 21, 2018
Merged

Support kAddTo for softmax backward#11836
anirudh2290 merged 1 commit intoapache:masterfrom
haojin2:softmax_addto

Conversation

@haojin2
Copy link
Contributor

@haojin2 haojin2 commented Jul 20, 2018

Description

Fix for issue #11411.

Checklist

Essentials

  • 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

  • kAddTo in backward pass

Comments

Note: No need to add kAddTo support for forward pass, actually there's no way to specify req to be kAddTo for forward computation.
New version of test_new_softmax passed more than 10000 trials on both CPU & GPU:

MXNET_TEST_COUNT=10000 nosetests --verbose tests/python/unittest/test_operator.py:test_new_softmax
/home/ubuntu/anaconda3/lib/python3.6/site-packages/urllib3/contrib/pyopenssl.py:46: DeprecationWarning: OpenSSL.rand is deprecated - you should use os.urandom instead
  import OpenSSL.SSL
/home/ubuntu/anaconda3/lib/python3.6/site-packages/nose/util.py:453: DeprecationWarning: inspect.getargspec() is deprecated, use inspect.signature() or inspect.getfullargspec()
  inspect.getargspec(func)
[INFO] Setting module np/mx/python random seeds, use MXNET_MODULE_SEED=1585858917 to reproduce.
test_operator.test_new_softmax ... ok

----------------------------------------------------------------------
Ran 1 test in 737.356s

OK
MXNET_TEST_COUNT=10000 nosetests --verbose tests/python/gpu/test_operator_gpu.py:test_new_softmax
/home/ubuntu/anaconda3/lib/python3.6/site-packages/urllib3/contrib/pyopenssl.py:46: DeprecationWarning: OpenSSL.rand is deprecated - you should use os.urandom instead
  import OpenSSL.SSL
/home/ubuntu/anaconda3/lib/python3.6/site-packages/nose/util.py:453: DeprecationWarning: inspect.getargspec() is deprecated, use inspect.signature() or inspect.getfullargspec()
  inspect.getargspec(func)
[INFO] Setting module np/mx/python random seeds, use MXNET_MODULE_SEED=1970091674 to reproduce.
test_operator_gpu.test_new_softmax ... ok

----------------------------------------------------------------------
Ran 1 test in 904.984s

OK

@haojin2
Copy link
Contributor Author

haojin2 commented Jul 20, 2018

Experiencing some bugs on GPU side, changing to WIP.

@haojin2 haojin2 changed the title Support kAddTo for softmax [WIP] Support kAddTo for softmax Jul 20, 2018
@eric-haibin-lin
Copy link
Member

any update?

@haojin2
Copy link
Contributor Author

haojin2 commented Aug 6, 2018

@eric-haibin-lin There's a weird bug that I'm still investigating...

@sandeep-krishnamurthy sandeep-krishnamurthy added Operator Backend Issues related to the backend of MXNet pr-work-in-progress PR is still work in progress labels Aug 8, 2018
@haojin2 haojin2 changed the title [WIP] Support kAddTo for softmax Support kAddTo for softmax backward Aug 16, 2018
expected_fwd = np_softmax(data, axis=axis)
expected_bwd = np.zeros(shape)
check_symbolic_forward(sym, [data], [expected_fwd])
for req in ['add', 'write']:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also add 'null' to the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@apeforest
Copy link
Contributor

@haojin2 I am curious what the bug you experienced earlier was and how did you fix it? I only see one commit in this PR. Thanks!

@haojin2
Copy link
Contributor Author

haojin2 commented Aug 17, 2018

@apeforest it was only some typo

@apeforest
Copy link
Contributor

Thanks for clarifying. LGTM.

@apeforest
Copy link
Contributor

@anirudh2290 @sandeep-krishnamurthy Can you help merge this PR at your convenience? Thanks


if (req_grad) {
mxnet_op::SoftmaxGrad<mshadow_op::mul, mxnet_op::softmax_bwd>(s,
mxnet_op::SoftmaxGrad<mshadow_op::mul, mxnet_op::softmax_bwd, kWriteTo>(s,
Copy link
Member

Choose a reason for hiding this comment

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

do we want to reenable cudnn_forward for ctcloss op ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not within the scope of this PR, this PR is purely for fixing the problem with the request type the backward of softmax can handle. I can probably take a look into the problem later though, but I think this PR should be good to merge.

@anirudh2290 anirudh2290 merged commit d7b39f4 into apache:master Aug 21, 2018
@haojin2 haojin2 deleted the softmax_addto branch August 22, 2018 16:56
XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Sep 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Backend Issues related to the backend of MXNet Operator pr-work-in-progress PR is still work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants