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

Added large tensor support and test for gather_nd #16371

Merged
merged 2 commits into from
Oct 21, 2019
Merged

Added large tensor support and test for gather_nd #16371

merged 2 commits into from
Oct 21, 2019

Conversation

access2rohit
Copy link
Contributor

@access2rohit access2rohit commented Oct 3, 2019 •
edited
Loading

Description

changed the operator code to use index_t instead of int

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)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Testing

Currently running full test suite. Will update once done.

@access2rohit
Copy link
Contributor Author

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

@lanking520 lanking520 added the pr-work-in-progress PR is still work in progress label Oct 3, 2019
@access2rohit access2rohit changed the title [WIP]Adding large tensor support and test for gather_nd Adding large tensor support and test for gather_nd Oct 4, 2019
@access2rohit access2rohit changed the title Adding large tensor support and test for gather_nd [WIP]Adding large tensor support and test for gather_nd Oct 4, 2019
@access2rohit
Copy link
Contributor Author

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

@lanking520 lanking520 added the pr-awaiting-review PR is waiting for code review label Oct 4, 2019
@access2rohit
Copy link
Contributor Author

@ChaiBapchya @apeforest This PR is ready for review

access2rohit
access2rohit commented Oct 4, 2019
View reviewed changes
python/mxnet/ndarray/ndarray.py
Comment on lines +1051 to +1056
if sys.version_info[0] > 2 and _int64_enabled():
idx_dtype = 'int64'
else:
idx_dtype = 'int32'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Performing this check since idx can be an instance of NDArray, list, tuple, slice(python), integer_type etc. Also, earlier idx was hard coded to be 'int32' therefore with Large Tensor enabled it makes sense to use only 'int64'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we measured runtime difference by adding this check in ndarray.py? Can we some simple test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ran test_ndarray.py on my branch and on master. Both take 20-21 secs. Master is taking 1-2 secs more than my change.

@access2rohit access2rohit changed the title [WIP]Adding large tensor support and test for gather_nd Adding large tensor support and test for gather_nd Oct 4, 2019
apeforest
apeforest reviewed Oct 4, 2019
View reviewed changes
src/operator/tensor/indexing_op.h Outdated
int offset = 0;
for (int j = 0; j < M; ++j) {
index_t offset = 0;
for (index_t j = 0; j < M; ++j) {
offset += strides[j] * static_cast<int>(indices[j*N + i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would static_cast cause the same overflow problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad. it should be cast to index_t

apeforest
apeforest reviewed Oct 4, 2019
View reviewed changes
tests/nightly/test_large_array.py
@@ -1212,6 +1212,15 @@ def test_full():
assert a[-1][-1] == 3


def test_gather():
arr = mx.nd.ones((LARGE_X, SMALL_Y))
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need nested parentheses right??

Copy link
Contributor

@ChaiBapchya ChaiBapchya Oct 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We do need parentheses

mx.nd.ones(3,4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/bapac/workspace/transpose/incubator-mxnet/python/mxnet/ndarray/ndarray.py", line 3153, in ones
    return _internal._ones(shape=shape, ctx=ctx, dtype=dtype, **kwargs)
  File "<string>", line 36, in _ones
  File "/Users/bapac/workspace/transpose/incubator-mxnet/python/mxnet/_ctypes/ndarray.py", line 100, in _imperative_invoke
    ctypes.byref(out_stypes)))
  File "/Users/bapac/workspace/transpose/incubator-mxnet/python/mxnet/base.py", line 254, in check_call
    raise MXNetError(py_str(_LIB.MXGetLastError()))
mxnet.base.MXNetError: [11:15:17] ../include/mxnet/./base.h:526: Invalid context string 4

apeforest
apeforest reviewed Oct 4, 2019
View reviewed changes
tests/nightly/test_large_array.py Outdated
@@ -1212,6 +1212,15 @@ def test_full():
assert a[-1][-1] == 3


def test_gather():
arr = mx.nd.ones((LARGE_X, SMALL_Y))
idx = mx.nd.random.randint(0, LARGE_X, (SMALL_X), 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.

remove parenthesis around SMALL_X

@access2rohit access2rohit changed the title Adding large tensor support and test for gather_nd Added large tensor support and test for gather_nd Oct 16, 2019
@access2rohit
Copy link
Contributor Author

@sxjscience @apeforest @anirudh2290 @zheng-da @pengzhao-intel This PR is ready for review

@access2rohit access2rohit force-pushed the scatter_lts branch 2 times, most recently from bd9a7a6 to 51c5386 Compare October 17, 2019 18:22
@access2rohit
Copy link
Contributor Author

`
test_large_array.test_gather ... ok

Ran 1 test in 3.056s

OK
`

anirudh2290
anirudh2290 reviewed Oct 17, 2019
View reviewed changes
tests/nightly/test_large_array.py Outdated
@@ -1199,6 +1199,15 @@ def test_full():
assert a[-1][-1] == 3


def test_gather():
arr = mx.nd.ones((LARGE_X, SMALL_Y))
idx = mx.nd.random.randint(0, LARGE_X, SMALL_X, dtype=np.int64)
Copy link
Member

Choose a reason for hiding this comment

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

some comment here on the test will help.

Copy link
Contributor Author

@access2rohit access2rohit Oct 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

done. added test on large vector as well which i missed in previous commit. Thanks for pointing that out

@access2rohit access2rohit force-pushed the scatter_lts branch 6 times, most recently from 9dc433f to e27758a Compare October 18, 2019 21:51
@anirudh2290 anirudh2290 merged commit 62b0638 into apache:master Oct 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apeforest apeforest apeforest left review comments

@ChaiBapchya ChaiBapchya ChaiBapchya approved these changes

@anirudh2290 anirudh2290 anirudh2290 approved these changes

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

Assignees
No one assigned
Labels
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.
5 participants
@access2rohit @anirudh2290 @apeforest @ChaiBapchya @lanking520