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/08/25 23:39:08 UTC

[GitHub] [incubator-mxnet] Zha0q1 opened a new pull request #19013: Numpy pooling fix

Zha0q1 opened a new pull request #19013:
URL: https://github.com/apache/incubator-mxnet/pull/19013


   This PR adds large dimension checks to pooling and roi pooling. Those operators should not have large dimension use cases but we add in the checks just to be safe. Also I updated the large tensor tests for the two operators


----------------------------------------------------------------
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 #19013: Numpy Pooling and ROI Pooling Large Dimension Checks

Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on pull request #19013:
URL: https://github.com/apache/incubator-mxnet/pull/19013#issuecomment-680375555


   > Thank you for the fix! I think some hard code should be replaced.
   
   Sure. Thanks for reviewing


----------------------------------------------------------------
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 #19013: Numpy pooling fix

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19013:
URL: https://github.com/apache/incubator-mxnet/pull/19013#issuecomment-680321177


   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-cpu, windows-gpu, centos-gpu, sanity, edge, unix-gpu, unix-cpu, website, clang, miscellaneous, centos-cpu]
   *** 
   _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] wkcn commented on a change in pull request #19013: Numpy Pooling and ROI Pooling Large Dimension Checks

Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #19013:
URL: https://github.com/apache/incubator-mxnet/pull/19013#discussion_r476918680



##########
File path: src/operator/roi_pooling-inl.h
##########
@@ -177,6 +177,11 @@ class ROIPoolingProp : public OperatorProperty {
     mxnet::TShape dshape = in_shape->at(roipool::kData);
     CHECK_EQ(dshape.ndim(), 4U) << "data should be a 4D tensor";
 
+    for (int i=0; i<dshape.ndim(); i++) {
+        CHECK_LT(dshape[i], (int64_t{1} << 31) - 1) << 

Review comment:
       We can replace `(int64_t{1} << 31) - 1` with `INT32_MAX`.




----------------------------------------------------------------
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] szha merged pull request #19013: Numpy Pooling and ROI Pooling Large Dimension Checks

Posted by GitBox <gi...@apache.org>.
szha merged pull request #19013:
URL: https://github.com/apache/incubator-mxnet/pull/19013


   


----------------------------------------------------------------
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] wkcn commented on a change in pull request #19013: Numpy Pooling and ROI Pooling Large Dimension Checks

Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #19013:
URL: https://github.com/apache/incubator-mxnet/pull/19013#discussion_r476920915



##########
File path: tests/nightly/test_np_large_array.py
##########
@@ -1005,33 +1006,44 @@ def test_dlpack():
     assert C[0][100] == 101
 
 @use_np
-@pytest.mark.skip(reason='broken on large tensors')
-#TODO add 3d pooling test after large tensor is fixed
 def test_pooling():
-    A = np.ones((1, 2, INT_OVERFLOW))
-    A[0][0][2] = 100
+    def test_pooling_large_dim():
+        A = np.ones((1, 1, INT_OVERFLOW))
+        assertRaises(MXNetError, npx.pooling, data=A, kernel=(2), stride=(2), \
+                pool_type='max')
+    
+    test_pooling_large_dim()
+    A = np.ones((1, 1, 2**12, 2**10, 2**10))

Review comment:
       Can you replace the hard code number with a variable?
   For example, 
   ```python
   C, H, W = 2**12, 2**10, 2**10
   A = np.ones((1, 1, C, H, W))
   ```




----------------------------------------------------------------
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] wkcn commented on a change in pull request #19013: Numpy Pooling and ROI Pooling Large Dimension Checks

Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #19013:
URL: https://github.com/apache/incubator-mxnet/pull/19013#discussion_r476918170



##########
File path: src/operator/nn/pooling.cc
##########
@@ -117,6 +117,11 @@ static bool PoolingShape(const nnvm::NodeAttrs &attrs,
       << " Or 4D in (batch, channel, y, x) "
       << " Or 5D in (batch, channel, d, y, x)";
 
+  for (int i=0; i<dshape.ndim(); i++) {
+    CHECK_LT(dshape[i], (int64_t{1} << 31) - 1) << 

Review comment:
       We can replace `(int64_t{1} << 31) - 1` with `INT32_MAX`.




----------------------------------------------------------------
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