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/01/03 08:09:07 UTC
[GitHub] [incubator-mxnet] Alicia1529 opened a new pull request #17208:
gather_nd: check bound and wrap negative indices
Alicia1529 opened a new pull request #17208: gather_nd: check bound and wrap negative indices
URL: https://github.com/apache/incubator-mxnet/pull/17208
1. check if indices are out of bound
2. wrap negative indices
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] eric-haibin-lin commented on issue #17208:
gather_nd: check bound and wrap negative indices
Posted by GitBox <gi...@apache.org>.
eric-haibin-lin commented on issue #17208: gather_nd: check bound and wrap negative indices
URL: https://github.com/apache/incubator-mxnet/pull/17208#issuecomment-573936800
@szha we should not mix commits in a PR. It makes it hard to search for the commit that fixes " NormalizeError" APIs
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] haojin2 commented on issue #17208: gather_nd:
check bound and wrap negative indices
Posted by GitBox <gi...@apache.org>.
haojin2 commented on issue #17208: gather_nd: check bound and wrap negative indices
URL: https://github.com/apache/incubator-mxnet/pull/17208#issuecomment-571351466
Maybe also add some tests to verify this new checking.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] haojin2 merged pull request #17208: gather_nd:
check bound and wrap negative indices
Posted by GitBox <gi...@apache.org>.
haojin2 merged pull request #17208: gather_nd: check bound and wrap negative indices
URL: https://github.com/apache/incubator-mxnet/pull/17208
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] haojin2 commented on a change in pull request
#17208: gather_nd: check bound and wrap negative indices
Posted by GitBox <gi...@apache.org>.
haojin2 commented on a change in pull request #17208: gather_nd: check bound and wrap negative indices
URL: https://github.com/apache/incubator-mxnet/pull/17208#discussion_r364397061
##########
File path: tests/python/unittest/test_operator.py
##########
@@ -7166,6 +7166,35 @@ def check(data, idx):
idx = mx.nd.array([[0, 0, 0, 0]], dtype='int32')
assert (mx.nd._internal._backward_gather_nd(data, idx, shape=(1,)).asscalar() == data.asnumpy().sum())
+@with_seed()
+def test_gather_nd_check_bound():
+ # check if indices is out of bound
+ data = mx.nd.array([[0, 1, 2], [3, 4, 5]])
+ indices1 = mx.nd.array([[0, 1, 0], [0, 1, 3]])
+ indices2 = mx.nd.array([[0, 1, 0], [0, 1, -5]])
+ try:
+ mx.nd.gather_nd(data, indices1)
+ mx.nd.waitall()
+ except IndexError:
+ # skip errors since the test is supposed to raise error
+ # IndexError: index 3 is out of bounds for axis 1 with size 3
+ pass
+
+ try:
+ mx.nd.gather_nd(data, indices2)
+ mx.nd.waitall()
+ except IndexError:
+ # skip errors since the test is supposed to raise error
+ # IndexError: index -5 is out of bounds for axis 1 with size 3
+ pass
+
+ # check if the negative indices are wrapped correctly
+ indices1 = mx.nd.array([[0, 1, -1], [0, 1, -2]])
+ indices2 = mx.nd.array([[0, 1, 1], [0, 1, 1]])
+ data1 = mx.nd.gather_nd(data, indices1)
+ data2 = mx.nd.gather_nd(data, indices2)
+ assert_almost_equal(data1, data2, rtol=1e-5, atol=1e-5)
+
Review comment:
one more blank line below.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] haojin2 commented on a change in pull request
#17208: gather_nd: check bound and wrap negative indices
Posted by GitBox <gi...@apache.org>.
haojin2 commented on a change in pull request #17208: gather_nd: check bound and wrap negative indices
URL: https://github.com/apache/incubator-mxnet/pull/17208#discussion_r363521450
##########
File path: src/operator/tensor/indexing_op.cu
##########
@@ -437,6 +436,98 @@ inline void SparseEmbeddingOpBackwardRspImpl<gpu>(const bool deterministic,
});
}
+struct gather_nd {
+ template<typename DType, typename IType>
+ MSHADOW_XINLINE static void Map(index_t i, OpReqType req, index_t N, index_t M, index_t K,
+ const mshadow::Shape<10> strides,
+ const mshadow::Shape<10> mshape,
+ DType* out, const DType* data,
+ const IType* indices) {
+ index_t offset = 0;
+ for (index_t j = 0; j < M; ++j) {
+ offset += strides[j] * ((static_cast<index_t>(indices[j*N + i]) + mshape[j])%mshape[j]);
+ }
+ for (index_t j = 0; j < K; ++j) {
+ KERNEL_ASSIGN(out[i*K + j], req, data[offset+j]);
+ }
+ }
+};
+
+/*! \brief If there are out-of-bound indices, out will be assigned to 1.
+ */
+struct is_valid_check_gather_nd {
+ template<typename DType>
+ MSHADOW_XINLINE static void Map(int i, char* out, const DType* data,
+ const index_t N, const mshadow::Shape<10> mshape) {
+ index_t n = N - 1;
+ while (n >= 0) {
+ if (data[i*N + n] < -mshape[i] || data[i*N + n] > mshape[i] - 1) *out = 1;
Review comment:
You can already break when this if branch evaluates to `True`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] reminisce commented on a change in pull request
#17208: gather_nd: check bound and wrap negative indices
Posted by GitBox <gi...@apache.org>.
reminisce commented on a change in pull request #17208: gather_nd: check bound and wrap negative indices
URL: https://github.com/apache/incubator-mxnet/pull/17208#discussion_r363435410
##########
File path: src/operator/tensor/indexing_op.cu
##########
@@ -437,6 +436,98 @@ inline void SparseEmbeddingOpBackwardRspImpl<gpu>(const bool deterministic,
});
}
+struct gather_nd {
+ template<typename DType, typename IType>
+ MSHADOW_XINLINE static void Map(index_t i, OpReqType req, index_t N, index_t M, index_t K,
+ const mshadow::Shape<10> strides,
+ const mshadow::Shape<10> mshape,
+ DType* out, const DType* data,
+ const IType* indices) {
+ index_t offset = 0;
+ for (index_t j = 0; j < M; ++j) {
+ offset += strides[j] * ((static_cast<index_t>(indices[j*N + i]) + mshape[j])%mshape[j]);
+ }
+ for (index_t j = 0; j < K; ++j) {
+ KERNEL_ASSIGN(out[i*K + j], req, data[offset+j]);
+ }
+ }
+};
+
+/*! \brief If there are out-of-bound indices, out will be assigned to 1.
+ */
+struct is_valid_check_gather_nd {
+ template<typename DType>
+ MSHADOW_XINLINE static void Map(int i, char* out, const DType* data,
+ const index_t N, const mshadow::Shape<10> mshape) {
+ index_t n = N - 1;
+ while (n >= 0) {
+ if (data[i*N + n] < -mshape[i] || data[i*N + n] > mshape[i] - 1) *out = 1;
+ n--;
+ }
+ }
+};
+
+/*
+ * \brief returns true if all indices is out of bounds for axis x with size y
+ * \param s the stream
+ * \param data_ptr the indices on the stream
+ * \param data_size the number of indices to examine
+ * \param M the number of axises to exmaine
+ * \param mshape the array that stores shape for each dimension
+ * \param is_valid_ptr the temparary workspace
+ */
+template<typename DType>
+bool GatherNDCheckBound(mshadow::Stream<gpu> *s, const DType* data_ptr, index_t N,
+ index_t M, const mshadow::Shape<10> mshape, char* is_valid_ptr) {
+ using namespace mxnet_op;
+ int32_t is_valid = 0;
+ Kernel<set_zero, gpu>::Launch(s, 1, is_valid_ptr);
+ Kernel<is_valid_check_gather_nd, gpu>::Launch(s, M, is_valid_ptr, data_ptr, N, mshape);
+ CUDA_CALL(cudaMemcpyAsync(&is_valid, is_valid_ptr, sizeof(char),
+ cudaMemcpyDeviceToHost, mshadow::Stream<gpu>::GetStream(s)));
+ CUDA_CALL(cudaStreamSynchronize(mshadow::Stream<gpu>::GetStream(s)));
+ return is_valid == 0;
+}
+
+template<typename gpu>
+void GatherNDForward(const nnvm::NodeAttrs& attrs,
Review comment:
Same here. `GatherNDForwardGPU`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] reminisce commented on a change in pull request
#17208: gather_nd: check bound and wrap negative indices
Posted by GitBox <gi...@apache.org>.
reminisce commented on a change in pull request #17208: gather_nd: check bound and wrap negative indices
URL: https://github.com/apache/incubator-mxnet/pull/17208#discussion_r363435292
##########
File path: src/operator/tensor/indexing_op.cc
##########
@@ -435,6 +435,63 @@ inline void SparseEmbeddingOpBackwardRspImpl<cpu>(const bool deterministic,
});
}
+struct gather_nd {
+ template<typename DType, typename IType>
+ MSHADOW_XINLINE static void Map(index_t i, OpReqType req, index_t N, index_t M, index_t K,
+ const mshadow::Shape<10> strides,
+ DType* out, const DType* data,
+ const IType* indices) {
+ index_t offset = 0;
+ for (index_t j = 0; j < M; ++j) {
+ offset += strides[j] * static_cast<index_t>(indices[j*N + i]);
+ }
+ for (index_t j = 0; j < K; ++j) {
+ KERNEL_ASSIGN(out[i*K + j], req, data[offset+j]);
+ }
+ }
+};
+
+template<typename cpu>
+void GatherNDForward(const nnvm::NodeAttrs& attrs,
Review comment:
No need to use template function. You can name this `GatherNDForwardCPU`.
----------------------------------------------------------------
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
With regards,
Apache Git Services