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

adding test for softmax operator for inputs with large magnitude#13328

Merged
anirudh2290 merged 6 commits intoapache:masterfrom
mseth10:fix_13141
Nov 22, 2018
Merged

adding test for softmax operator for inputs with large magnitude#13328
anirudh2290 merged 6 commits intoapache:masterfrom
mseth10:fix_13141

Conversation

@mseth10
Copy link
Copy Markdown
Contributor

@mseth10 mseth10 commented Nov 20, 2018 •
edited
Loading

Description

Adding test for mkldnn and non-mkldnn Softmax operator for inputs with extremely large magnitudes. This PR fixes #13141 .

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

@stu1130
Copy link
Copy Markdown
Contributor

stu1130 commented Nov 20, 2018

@mxnet-label-bot add [pr-awaiting-review]
Thanks for your contribution @mseth10

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Nov 20, 2018
stu1130
stu1130 reviewed Nov 20, 2018
View reviewed changes
Comment thread tests/python/mkl/test_mkldnn.py Outdated
pengzhao-intel
pengzhao-intel suggested changes Nov 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

It's better we can cover more corner cases in this PR :)

pengzhao-intel
pengzhao-intel reviewed Nov 20, 2018
View reviewed changes
Comment thread tests/python/mkl/test_mkldnn.py Outdated
check_fullyconnected_training(stype)

def test_softmax_with_large_negative_inputs():
input_data = mx.nd.array([[[[-1e30,-1e30]]]])
Copy link
Copy Markdown
Contributor

@pengzhao-intel pengzhao-intel Nov 20, 2018

Choose a reason for hiding this comment

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

Could you build other corner cases to cover more, such as 1e-30 as input or other possible combinations?

Copy link
Copy Markdown
Contributor Author

@mseth10 mseth10 Nov 21, 2018

Choose a reason for hiding this comment

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

Added more corner cases: extremely large positive and negative inputs covered (also checked for max and min values of float32 inputs). Please suggest if you think there are more corner cases that should be added.

@mseth10 mseth10 changed the title adding test for mkldnn softmax operator for large negative inputs adding test for softmax operator for large negative inputs Nov 21, 2018
anirudh2290
anirudh2290 reviewed Nov 21, 2018
View reviewed changes
Comment thread tests/python/unittest/test_operator.py Outdated
exec1.forward()[0].wait_to_read()
ndarr = exec1.outputs[0][0][0][0]
nparr = ndarr.asnumpy()
assert np.array_equal(nparr, true_output)
Copy link
Copy Markdown
Member

@anirudh2290 anirudh2290 Nov 21, 2018

Choose a reason for hiding this comment

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

can we use assert_almost_equal in test_utils.

Copy link
Copy Markdown
Contributor Author

@mseth10 mseth10 Nov 21, 2018

Choose a reason for hiding this comment

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

assert_almost_equal makes more sense as we are comparing floats here. Corrected.

pengzhao-intel
pengzhao-intel approved these changes Nov 21, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@pengzhao-intel pengzhao-intel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM after all CI pass :)

Thanks for your help.

@mseth10 mseth10 changed the title adding test for softmax operator for large negative inputs adding test for softmax operator for inputs with large magnitude Nov 21, 2018
stu1130
stu1130 approved these changes Nov 21, 2018
View reviewed changes
Copy link
Copy Markdown
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!
Thanks @mseth10

@anirudh2290 anirudh2290 merged commit be76f85 into apache:master Nov 22, 2018
@mseth10 mseth10 deleted the fix_13141 branch November 26, 2018 20:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@anirudh2290 anirudh2290 anirudh2290 approved these changes

@stu1130 stu1130 stu1130 approved these changes

@KellenSunderland KellenSunderland KellenSunderland approved these changes

@pengzhao-intel pengzhao-intel pengzhao-intel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

MKLDNN softmax outputs NaN in mkldnn 0.14

6 participants

@mseth10 @stu1130 @anirudh2290 @KellenSunderland @pengzhao-intel @marcoabreu