operator MaxPool output tensor shape is ambiguously documented.

[srohit0]

Bug Report

Describe the bug

Operator: MaxPool
When attr ceil_mode is used with attr strides, it can produce more than one output_spatial_shape depending on application of complete/incomplete application of kernel or by changing the padding. MaxPool document is silent on the behavior in that case.

Example:

Consider following input tensor X with given attributes
X shape: 4x4
kernel_shape: 3x3
strides: 2x2
ceil_mode: 1

According to the equation given in operator documention (produced below)
output_spatial_shape[i] = ceil((input_spatial_shape[i] + pad_shape[i] - kernel_spatial_shape[i]) / strides_spatial_shape[i] + 1)
is equal to 2x2. But kernel 3x3 can't be complete applied, since it goes out of the input X's shape.

Case 1: Resulting output spatial shape while, honoring padding and complete kernel application: 1x1
Case 2: Resulting output spatial shape while, honoring padding and partial kernel application: 2x2
Case 3: Resulting output spatial shape while, changed padding (from 0,0 to 1,1) and complete kernel application: 2x2

Document must be clear on the behavior, since it affects resulting output tensor and its elements values.

System information

• Linux Ubuntu 18.04:
• 1.5
• Python version: 3.6.9
• GCC/Compiler version (if compiling from source): NA
• CMake version: NA
• Protobuf version: 3.5.1
• Visual Studio version (if applicable): NA

Reproduction instructions

• Describe the code to reproduce the behavior.

import numpy as np
import onnx
import future_onnx_backend; # future onnx backend
 
np_x = np.array([[[ [1, 2, 3, 4], [5, 6, 7, 8], [9, 10, 11, 12], [13, 14, 15, 16], ]]]).astype(np.float32)
model=onnx.load('test_MaxPool_2D_ceil.onnx')
rep = future_onnx_backend.prepare(model, device="CPU")
output = rep.run(np_x)

• Attach the ONNX model to the issue (where applicable)

ir_version: 5
graph {
  node {
    input: "0"
    output: "1"
    op_type: "MaxPool"
    attribute {
      name: "ceil_mode"
      i: 1
      type: INT
    }
    attribute {
      name: "kernel_shape"
      ints: 3
      ints: 3
      type: INTS
    }
    attribute {
      name: "strides"
      ints: 2
      ints: 2
      type: INTS
    }
  }
  name: "MaxPool_graph"
  input {
    name: "0"
    type {
      tensor_type {
        elem_type: 1
        shape {
          dim {
            dim_value: 1
          }
          dim {
            dim_value: 1
          }
          dim {
            dim_value: 4
          }
          dim {
            dim_value: 4
          }
        }
      }
    }
  }
  output {
    name: "1"
    type {
      tensor_type {
        elem_type: 1
        shape {
          dim {
            dim_value: 1
          }
          dim {
            dim_value: 1
          }
          dim {
            dim_value: 2
          }
          dim {
            dim_value: 2
          }
        }
      }
    }
  }
}
opset_import {
  version: 11
}

Expected behavior

Operator doc must clarify application of partial/full kernel or modified padding or honor ceil_mode attriute.

Notes

This bug applies to other operators where output_spatial_shape depends on ceil_mode and kernel_shape

[kraiskil]

Is there any update on this?

I guess since @faxu removed the 'bug' label, this means the 2x2 output size is still the desired correct one?

Also since this is what the back end tests for, it seems to be an implicit consensus among back end implementers that automatic padding should be used (despite the auto_pad attribute for maxpool being marked deprecated, and not used in the maxpool_2d_ceil backend test ).

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[srohit0]

Why was it closed. Where's the resolution?

[faxu]

Stalebot closes issues without activity. @srohit0 looks like this is already tracked in #3651 now - can you follow up there?

[srohit0]

@faxu , I see this bug mentioned there. Doesn't that mean it has to stay open.

Are you creating a one large documentation bug for all the operators?

[jcwchen]

@srohit0 Thanks for the reminder. I will keep this issue open to let people know it hasn't been fixed yet. stale robot might accidentally close some inactive issues which are still valid. I am trying to reopen them if they haven't been resolved yet. Reopening now.

[rajveer43]

Is it Open, If yes then I would work on it. Kindly give insights In which file what I have to change

[srohit0]

Is it Open, If yes then I would work on it. Kindly give insights In which file what I have to change

You can start from the documentation on which it was filed - https://github.com/onnx/onnx/blob/rel-1.5.0/docs/Operators.md#maxpool

LIkely more tests would be added to ensure new behavior.

[rajveer43]

Is it Open, If yes then I would work on it. Kindly give insights In which file what I have to change

You can start from the documentation on which it was filed - https://github.com/onnx/onnx/blob/rel-1.5.0/docs/Operators.md#maxpool

LIkely more tests would be added to ensure new behavior.

Okay, should I clone the master branch or the rel1.5.0 ?

[jcwchen]

Okay, should I clone the master branch or the rel1.5.0 ?

The main branch will be preferred. The document in this repo and the web version of document will be reflected right after the PR gets merged. Thank you for the contribution!

[rajveer43]

Sure @jcwchen

[justinchuby]

@titaiwangms @liqunfu was this fixed by #5741?