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

Added tests to verify Large Vector Support for initial set of ops #15943

Merged
merged 4 commits into from
Aug 27, 2019
Merged

Added tests to verify Large Vector Support for initial set of ops #15943

merged 4 commits into from
Aug 27, 2019

Conversation

access2rohit
Copy link
Contributor

@access2rohit access2rohit commented Aug 19, 2019 •
edited
Loading

Description

Added Large Index Support for Slice and Softmax Ops and verification of Large Index support in a couple of operators. Extracted Large Vector and Tensor creation helper methods into test_utils.py

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

Testing

MXNET_TEST_COUNT=1 nosetests --logging-level=DEBUG --verbose -s tests/nightly/test_large_vector.py

/home/ubuntu/anaconda3/lib/python3.6/site-packages/h5py/init.py:36: FutureWarning: Conversion of the second argument of issubdtype from float to np.floating is deprecated. In future, it will be treated as np.float64 == np.dtype(float).type.
from ._conv import register_converters as _register_converters
test_large_vector.test_slice ... ok
test_large_vector.test_ndarray_zeros ... ok
test_large_vector.test_ndarray_ones ... ok
test_large_vector.test_ndarray_random_uniform ... [DEBUG] Setting test np/mx/python random seeds, use MXNET_TEST_SEED=1698621242 to reproduce.

ok
test_large_vector.test_ndarray_random_randint ... [DEBUG] Setting test np/mx/python random seeds, use MXNET_TEST_SEED=787222743 to reproduce.
ok
test_large_vector.test_ndarray_empty ... ok
test_large_vector.test_elementwise ... ok
test_large_vector.test_reduce ... ok
test_large_vector.test_clip ... ok
test_large_vector.test_argmin ... ok
test_large_vector.test_take ... ok
test_large_vector.test_slice_assign ... ok
test_large_vector.test_expand_dims ... ok
test_large_vector.test_squeeze ... ok
test_large_vector.test_broadcast_div ... ok
test_large_vector.test_Dense ... ok
test_large_vector.test_argsort ... ok
test_large_vector.test_sort ... ok
test_large_vector.test_topk ... ok

Ran 19 tests in 9905.014s

OK

@access2rohit
Copy link
Contributor Author

@mxnet-label-bot add [pr-work-in-progress]

@marcoabreu marcoabreu added the pr-work-in-progress PR is still work in progress label Aug 19, 2019
@access2rohit access2rohit changed the title [WIP]Added new C_Apis to suport Large vectors and added tests for initial set of ops Added new C_Apis to suport Large vectors and added tests for initial set of ops Aug 20, 2019
@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 Aug 20, 2019
@access2rohit
Copy link
Contributor Author

@apeforest @ChaiBapchya this is now ready for review

@access2rohit access2rohit force-pushed the large_vector branch 3 times, most recently from f8d5c89 to 44b217b Compare August 20, 2019 22:59
@access2rohit access2rohit changed the title Added new C_Apis to suport Large vectors and added tests for initial set of ops Added tests to verify Large Vector Support for initial set of ops Aug 21, 2019
@access2rohit access2rohit force-pushed the large_vector branch 3 times, most recently from 229838a to 661424b Compare August 21, 2019 10:25
@access2rohit
Copy link
Contributor Author

@apeforest @ChaiBapchya pr is ready for review

ChaiBapchya
ChaiBapchya reviewed Aug 21, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated

def test_ndarray_ones():
a = nd.ones(shape=(LARGE_X))
assert a[-1][0] == 1
Copy link
Contributor

@ChaiBapchya ChaiBapchya Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's a vector. So [0] isn't needed (no 2nd dimension!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct ... it still worked though. Its not right to check in this way. Will correct it !

tests/nightly/test_large_vector.py Outdated
@with_seed()
def test_ndarray_random_uniform():
a = nd.random.uniform(shape=LARGE_X)
assert a[-1][0] != 0
Copy link
Contributor

Choose a reason for hiding this comment

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

same!

@ChaiBapchya ChaiBapchya mentioned this pull request Aug 21, 2019
Open
@ChaiBapchya
Copy link
Contributor

Made a note of the skipped tests in this issue. For easy tracking.

apeforest
apeforest reviewed Aug 21, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated
def test_clip():
a = nd.arange(0, LARGE_X)
res = nd.clip(a, a_min=100, a_max=1000)
assert np.sum(res[-1].asnumpy() == 1000) == 101
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this? Isn't np.sum(res[-1].asnumpy() == 1000) == 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aah ... copy paste error !

apeforest
apeforest reviewed Aug 21, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated
assert (indices_2d.asnumpy() == np.array(original_2d_indices)).all()


def create_large_vector(size, dtype=np.int64):
Copy link
Contributor

@apeforest apeforest Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Move this to the beginning the file before all the test methods if it is for generic use. Even better, put these util methods to a separate file.

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

@access2rohit access2rohit force-pushed the large_vector branch 5 times, most recently from c1f2cd0 to 21c13f2 Compare August 23, 2019 20:57
apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated
assert_almost_equal(output.asnumpy(), expected, atol=1e-3, rtol=1e-3)


def test_spacetodepth():
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need for 1D test?

apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated
assert_almost_equal(output.asnumpy(), expected, atol=1e-3, rtol=1e-3)

@with_seed()
def test_diag():
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need for 1D test?

apeforest
apeforest reviewed Aug 23, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated


@with_seed()
def test_ravel_multi_index():
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need for 1D test?

apeforest
apeforest suggested changes Aug 23, 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.

Reviewed. Please remove tests not for 1D vector. Also, please run through the entire test file and paste the output in PR.

@apache apache deleted a comment from access2rohit Aug 23, 2019
@access2rohit access2rohit force-pushed the large_vector branch 5 times, most recently from b753fc1 to 51e5d4b Compare August 24, 2019 15:39
apeforest
apeforest reviewed Aug 25, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated

def test_topk():
b = create_vector(size=LARGE_X)
k = nd.topk(b, k=10, axis=0, dtype=np.int64)
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be more consistent to rename it to ind?

apeforest
apeforest reviewed Aug 25, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated
assert np.sum(k.asnumpy() == (LARGE_X - 1)) == 1
ind, val = mx.nd.topk(b, k=3, axis=0, dtype=np.int64, ret_typ="both", is_ascend=False)
assert np.all(ind == val)
l = nd.topk(b, k=1, axis=0, dtype=np.int64, ret_typ="value")
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to val

apeforest
apeforest reviewed Aug 25, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated


def test_softmax():
input_data = nd.ones((2, LARGE_X))
Copy link
Contributor

Choose a reason for hiding this comment

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

1D?

apeforest
apeforest reviewed Aug 25, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated


def test_pick():
a = mx.nd.ones(shape=(LARGE_X, 2))
Copy link
Contributor

Choose a reason for hiding this comment

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

1D?

apeforest
apeforest reviewed Aug 25, 2019
View reviewed changes
tests/nightly/test_large_vector.py Outdated
a = nd.random.randint(low_large_value, high_large_value, dtype=np.int64)
low = mx.nd.array([low_large_value], dtype='int64')
high = mx.nd.array([high_large_value], dtype='int64')
assert a.__gt__(low) and a.__lt__(high)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just a > low and a < high?

@apeforest
Copy link
Contributor

Please paste your output here after you run through the test locally. Thanks!

@access2rohit access2rohit force-pushed the large_vector branch 2 times, most recently from 31b9038 to 154a343 Compare August 26, 2019 02:58
apeforest
apeforest approved these changes Aug 26, 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. Thanks for the quick response!

@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 Aug 27, 2019
@apeforest apeforest merged commit 0e71fbd into apache:master Aug 27, 2019
access2rohit added a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 25, 2019
@access2rohit
Added tests to verify Large Vector Support for initial set of ops (ap…
700dc1a 
…ache#15943)

* Adding tests to verify support for Large Tensors in additional Ops along with new C_Apis supporting 64bit indexing

* removing skipped tests

* enabling Large Index support for slice and softmax

* removing tests not required for vector testing
access2rohit added a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 25, 2019
@access2rohit
Added tests to verify Large Vector Support for initial set of ops (ap…
d1f5c42 
…ache#15943)

* Adding tests to verify support for Large Tensors in additional Ops along with new C_Apis supporting 64bit indexing

* removing skipped tests

* enabling Large Index support for slice and softmax

* removing tests not required for vector testing
access2rohit added a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 25, 2019
@access2rohit
Added tests to verify Large Vector Support for initial set of ops (ap…
7448111 
…ache#15943)

* Adding tests to verify support for Large Tensors in additional Ops along with new C_Apis supporting 64bit indexing

* removing skipped tests

* enabling Large Index support for slice and softmax

* removing tests not required for vector testing
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ChaiBapchya ChaiBapchya ChaiBapchya left review comments

@apeforest apeforest apeforest approved these changes

@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin

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

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 pr-work-in-progress PR is still work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@access2rohit @ChaiBapchya @apeforest @marcoabreu