You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/10/06 22:22:12 UTC
[GitHub] [incubator-mxnet] Zha0q1 opened a new pull request #19302: Numpy diag large tensor fix
Zha0q1 opened a new pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302
This pr fixes the four diag operators against large tensor
sample local run:
```
ubuntu@ip-172-31-38-169:~/incubator-mxnet$ pytest tests/nightly/test_np_large_array.py::test_diag
/home/ubuntu/anaconda3/lib/python3.7/importlib/_bootstrap.py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 from PyObject
return f(*args, **kwds)
/home/ubuntu/anaconda3/lib/python3.7/importlib/_bootstrap.py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 from PyObject
return f(*args, **kwds)
/home/ubuntu/anaconda3/lib/python3.7/importlib/_bootstrap.py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 from PyObject
return f(*args, **kwds)
================================================ test session starts ================================================
platform linux -- Python 3.7.7, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /home/ubuntu/incubator-mxnet, inifile: pytest.ini
plugins: remotedata-0.3.2, openfiles-0.4.0, astropy-header-0.1.2, hypothesis-5.8.3, arraydiff-0.3, doctestplus-0.5.0
collected 1 item
tests/nightly/test_np_large_array.py . [100%]
================================================= warnings summary ==================================================
tests/nightly/test_np_large_array.py:89
/home/ubuntu/incubator-mxnet/tests/nightly/test_np_large_array.py:89: DeprecationWarning: invalid escape sequence \
'''
tests/nightly/test_np_large_array.py:752
/home/ubuntu/incubator-mxnet/tests/nightly/test_np_large_array.py:752: DeprecationWarning: invalid escape sequence \
'''
-- Docs: https://docs.pytest.org/en/latest/warnings.html
========================================== 1 passed, 2 warnings in 59.45s ==========================================
``
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r500636767
##########
File path: src/api/operator/numpy/np_matrix_op.cc
##########
@@ -580,7 +583,10 @@ MXNET_REGISTER_API("_npi.diagonal")
const nnvm::Op* op = Op::Get("_npi_diagonal");
nnvm::NodeAttrs attrs;
op::NumpyDiagonalParam param;
- param.offset = args[1].operator int();
+ if (features::is_enabled(features::INT64_TENSOR_SIZE))
Review comment:
same as above
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r500652490
##########
File path: src/api/operator/numpy/np_matrix_op.cc
##########
@@ -490,7 +490,10 @@ MXNET_REGISTER_API("_npi.diag")
const nnvm::Op* op = Op::Get("_npi_diag");
nnvm::NodeAttrs attrs;
op::NumpyDiagParam param;
- param.k = args[1].operator int();
+ if (features::is_enabled(features::INT64_TENSOR_SIZE))
+ param.k = args[1].operator int64_t();
+ else
+ param.k = args[1].operator int();
Review comment:
it's already changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 closed pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 closed pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-705761635
Jenkins CI successfully triggered : [miscellaneous]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sxjscience commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r502722859
##########
File path: src/operator/numpy/np_matrix_op-inl.h
##########
@@ -1250,7 +1250,7 @@ void NumpyConcatenateBackward(const nnvm::NodeAttrs& attrs,
}
struct NumpyDiagParam : public dmlc::Parameter<NumpyDiagParam> {
- int k;
+ index_t k;
Review comment:
Given that we have added the support, it's fine to use index_t.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r500636668
##########
File path: src/api/operator/numpy/np_matrix_op.cc
##########
@@ -490,7 +490,10 @@ MXNET_REGISTER_API("_npi.diag")
const nnvm::Op* op = Op::Get("_npi_diag");
nnvm::NodeAttrs attrs;
op::NumpyDiagParam param;
- param.k = args[1].operator int();
+ if (features::is_enabled(features::INT64_TENSOR_SIZE))
+ param.k = args[1].operator int64_t();
+ else
+ param.k = args[1].operator int();
Review comment:
why not just use `index_t`? if enabled it will be int64 else int32
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-704675528
@access2rohit I resolved the issues
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-705761598
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r502670066
##########
File path: src/operator/numpy/np_matrix_op-inl.h
##########
@@ -1250,7 +1250,7 @@ void NumpyConcatenateBackward(const nnvm::NodeAttrs& attrs,
}
struct NumpyDiagParam : public dmlc::Parameter<NumpyDiagParam> {
- int k;
+ index_t k;
Review comment:
Maybe the user can have a (SMALL, VERY LARGE) tensor but yeah it's pretty unlikely a valid use case. Should I revert this then?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-705761598
@mxnet-bot run ci [miscellaneous]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sxjscience commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r502651151
##########
File path: src/operator/numpy/np_matrix_op-inl.h
##########
@@ -1250,7 +1250,7 @@ void NumpyConcatenateBackward(const nnvm::NodeAttrs& attrs,
}
struct NumpyDiagParam : public dmlc::Parameter<NumpyDiagParam> {
- int k;
+ index_t k;
Review comment:
Why change k to index_t?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sxjscience commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
sxjscience commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-706380068
Should we add the support of index_t to https://github.com/apache/incubator-mxnet/blob/master/include/mxnet/runtime/packed_func.h instead? Also cc @szha @sandeep-krishnamurthy
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r500651744
##########
File path: src/api/operator/numpy/np_matrix_op.cc
##########
@@ -490,7 +490,10 @@ MXNET_REGISTER_API("_npi.diag")
const nnvm::Op* op = Op::Get("_npi_diag");
nnvm::NodeAttrs attrs;
op::NumpyDiagParam param;
- param.k = args[1].operator int();
+ if (features::is_enabled(features::INT64_TENSOR_SIZE))
+ param.k = args[1].operator int64_t();
+ else
+ param.k = args[1].operator int();
Review comment:
change here: https://github.com/apache/incubator-mxnet/blob/master/src/operator/numpy/np_matrix_op-inl.h#L1253
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sxjscience merged pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
sxjscience merged pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-705810652
@access2rohit can you review again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-704584264
Hey @Zha0q1 , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [windows-gpu, edge, clang, unix-gpu, website, sanity, miscellaneous, unix-cpu, windows-cpu, centos-cpu, centos-gpu]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-706322428
@samskalicky would you help merge this thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 closed pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 closed pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sxjscience commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r502651401
##########
File path: src/operator/numpy/np_matrix_op-inl.h
##########
@@ -1250,7 +1250,7 @@ void NumpyConcatenateBackward(const nnvm::NodeAttrs& attrs,
}
struct NumpyDiagParam : public dmlc::Parameter<NumpyDiagParam> {
- int k;
+ index_t k;
Review comment:
I think `int k` should be good enough for most scenarios. There's no need to use `index_t` for k.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-706381338
> Should we add the support of index_t to https://github.com/apache/incubator-mxnet/blob/master/include/mxnet/runtime/packed_func.h instead? Also cc @szha @sandeep-krishnamurthy
I tried to do this but at compile time index_t becomes int/int64_t so we have a double definition error
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] access2rohit commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r500644524
##########
File path: tests/nightly/test_np_large_array.py
##########
@@ -665,6 +665,84 @@ def test_subtract():
assert B.grad.shape == (INT_OVERFLOW, 2)
assert B.grad[0][0] == -1
+@use_np
+def test_diag():
+ # test diag extraction
+ inp = np.zeros((2, INT_OVERFLOW+2))
+ inp[-1, -1] = 1
+ inp.attach_grad()
+ with mx.autograd.record():
+ out = np.diag(inp, k=INT_OVERFLOW)
+ out.backward()
+ assert out.shape == (2, )
+ assert out[1] == 1
+ assert inp.grad.shape == inp.shape
+ assert inp.grad[1, -1] == 1 and inp.grad[0, -2] == 1
+ # now test mat generation
+ N = 2**16
+ inp = np.ones((N))
+ inp[-1] = 2
+ inp.attach_grad()
+ with mx.autograd.record():
+ out = np.diag(inp)
+ out.backward()
+ assert out.shape == (N, N)
+ assert out[-1, -1] == 2 and out[0, 0] == 1
+ assert inp.grad.shape == inp.shape
+ assert inp.grad[-1] == 1
+
+@use_np
+def test_diag_indices_from():
+ N = 2**16
+ inp = np.zeros((N, N))
+ inp.attach_grad()
+ with mx.autograd.record():
+ dim1, dim2 = np.diag_indices_from(inp)
+ dim1.backward()
+ assert dim1.shape == (N, ) and dim2.shape == (N, )
+ assert dim1[-1] == N-1 and dim2[-1] == N-1
+ assert inp.grad.shape == inp.shape
+ assert inp[0, 0] == 0
+
+@use_np
+def test_diagflat():
+ N = 2**16
+ inp = np.ones((2, N))
+ inp[-1, -1] = 2
+ inp.attach_grad()
+ with mx.autograd.record():
+ out = np.diagflat(inp)
+ out.backward()
+ assert out.shape == (N*2, N*2)
+ assert out[-1, -1] == 2
Review comment:
can you add a zero check for any other element too ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] Zha0q1 commented on a change in pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#discussion_r500645129
##########
File path: src/api/operator/numpy/np_matrix_op.cc
##########
@@ -490,7 +490,10 @@ MXNET_REGISTER_API("_npi.diag")
const nnvm::Op* op = Op::Get("_npi_diag");
nnvm::NodeAttrs attrs;
op::NumpyDiagParam param;
- param.k = args[1].operator int();
+ if (features::is_enabled(features::INT64_TENSOR_SIZE))
+ param.k = args[1].operator int64_t();
+ else
+ param.k = args[1].operator int();
Review comment:
there is no index_t operator in include/mxnet/runtime/packed_func.h so we need to branch like this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19302: Numpy diag large tensor fix
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19302:
URL: https://github.com/apache/incubator-mxnet/pull/19302#issuecomment-705761635
Jenkins CI successfully triggered : [miscellaneous]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org