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

[MXNET-1405] tests for large tensor support for Softmax operator #15042

Merged
merged 1 commit into from
Jun 4, 2019
Merged

[MXNET-1405] tests for large tensor support for Softmax operator #15042

merged 1 commit into from
Jun 4, 2019

Conversation

access2rohit
Copy link
Contributor

@access2rohit access2rohit commented May 22, 2019 •
edited
Loading

Description

Added test case to verify Large Tensor Support for Softmax Operator

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-1405], where [MXNET-1405] tests for large tensor support for Softmax operator #15042 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

@access2rohit
Copy link
Contributor Author

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label May 22, 2019
@access2rohit
Copy link
Contributor Author

@apeforest : Please review

apeforest
apeforest reviewed May 23, 2019
View reviewed changes
tests/nightly/test_large_array.py Outdated
@@ -279,6 +279,19 @@ def test_diag():
assert_almost_equal(r.asnumpy(), np.diag(a_np, k=k))


def test_softmax():
def softmax_forward(input_data, true_output):
data = mx.sym.Variable('data')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use ndarray API?

apeforest
apeforest reviewed May 23, 2019
View reviewed changes
tests/nightly/test_large_array.py Outdated
nparr = ndarr.asnumpy()
assert_almost_equal(nparr, true_output, rtol=1e-5, atol=1e-5)

softmax_forward(mx.nd.ones((128, LARGE_X)), np.full((128, LARGE_X), 0.0078125))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this 0.0078125 hand calculated? How to verify it? Can you use numpy calculation as the expected result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes ... will do

apeforest
apeforest reviewed May 23, 2019
View reviewed changes
tests/nightly/test_large_array.py Outdated
nparr = ndarr.asnumpy()
assert_almost_equal(nparr, true_output, rtol=1e-5, atol=1e-5)

softmax_forward(mx.nd.ones((128, LARGE_X)), np.full((128, LARGE_X), 0.0078125))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 128? Can we use predefined constants?

apeforest
apeforest reviewed May 23, 2019
View reviewed changes
tests/nightly/test_large_array.py Outdated
@@ -279,6 +279,19 @@ def test_diag():
assert_almost_equal(r.asnumpy(), np.diag(a_np, k=k))


def test_softmax():
def softmax_forward(input_data, true_output):
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is called once, do we need this sub routine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@access2rohit access2rohit force-pushed the softmax branch 3 times, most recently from 355049f to 03aa04d Compare May 29, 2019 17:59
apeforest
apeforest reviewed May 29, 2019
View reviewed changes
tests/nightly/test_large_array.py
def test_softmax():
input_data = mx.nd.ones((SMALL_Y, LARGE_X))
true_output = np.full((SMALL_Y, LARGE_X), (1 / SMALL_Y))
output = nd.softmax(input_data, axis=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

when you test this way, the softmax is operating only on one dimension which does not exceed 2^32, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

But shouldn't we test softmax over a vector larger than 2^32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test fails

Copy link
Contributor

Choose a reason for hiding this comment

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

fine. let's leave that as TODO in the next phase to support large int in one dimension.

apeforest
apeforest approved these changes May 31, 2019
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

LGTM

@access2rohit
Copy link
Contributor Author

@mxnet-label-bot add [pr-awaiting-merge]

@marcoabreu marcoabreu added the pr-awaiting-merge Review and CI is complete. Ready to Merge label May 31, 2019
@access2rohit access2rohit force-pushed the softmax branch 4 times, most recently from 15301a2 to f523f73 Compare June 2, 2019 22:14
@apeforest apeforest merged commit a37cd7a into apache:master Jun 4, 2019
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@access2rohit
[MXNET-1405] tests for large tensor support for Softmax operator (apa…
612f2c3 
…che#15042)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apeforest apeforest apeforest approved these changes

Assignees
No one assigned
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge 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.
3 participants
@access2rohit @apeforest @marcoabreu