Inconsistencies between MaxPool, AveragePool, and LpPool specs

[JamesAllingham]

The MaxPool op has a few attributes that neither AveragePool nor LpPool have (dilations and storage_order). It isn't clear to me why these attributes should be exclusive to MaxPool. Though, to be honest, I'm not really sure what storage_order is for at all :P.

Similarly, LpPool only has auto_pad, kernel_size, and strides in common with AveragePool and MaxPool. It is not clear to me why ceil_mode, and pads should not apply here too.

(On the other hand, I do see why the count_include_pad attribute of AveragePool and the p attribute of LpPool are specific to their ops.)

Can anyone provide some insight here?

[ebarsoum]

storage_order specify if the tensor format is row major or column major, this shouldn't be part of the spec.
Regarding dilation, because if you compute average or norm, should you count the skipped cells or not? We can add an attribute similar to count_include_pad for that, but I am not aware of any model that use dilation with average and LpPool.

[kraiskil]

I wonder if the storage_order could be removed? Or at least deprecated? It feels like an unnecessary pain to implement in a backend.

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

[jcwchen]

Reopen it since this issue still exists.

[p-wysocki]

I'm working on it.

[p-wysocki]

Related PRs: #4534 and #4533.

[gramalingam]

As a clarification: the storage_order is used in MaxPool to convert the n-tuple of indices (i0, i1, i2, ...) into a single integer for its second output. Since the other pooling ops don't have such an output, it is not relevant for them. It could have been avoided if the output returned the n-tuple of indices (which is what an op like NonZero does). But it's not worth making a compatibility-breaking change to the op at this point.

[p-wysocki]

Most likely final PR (WIP at the time of writing): #4790

Alignment after it gets merged:

[p-wysocki]

The last PR has been merged. The current alignment:

Please reopen if needed.