Add MaxPool-18 - remove storage_order attribute
#4533
Conversation
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
| @@ -186,7 +186,7 @@ def export_maxpool_2d_precomputed_strides() -> None: | |||
| ) | |||
|
|
|||
| @staticmethod | |||
| def export_maxpool_with_argmax_2d_precomputed_strides() -> None: | |||
| def export_maxpool_with_argmax_2d_precomputed_strides_opset12() -> None: | |||
There was a problem hiding this comment.
Does the storage_order attribute affect the output-value? I assume it has no effect on the output, is that correct? Just trying to make sure I understand. If it has an effect, then the adapter needs to be different.
There was a problem hiding this comment.
I don't see how the storage order should affect on the python code level. Also looking at the .pb tensors files generated out of this test (both input and output), the data seems to be plain-old-row-major. (which makes sense - grepping for storage_order in the onnx sources shows it is nowhere implemented).
But indeed, the output is affected - the optional output-indices (tensor z) calculated in this test seem to have indices calculated in a column-major fashion, and z needs transposing for the test to be correct.
There was a problem hiding this comment.
Should z be corrected if this test is for opset12 only? The operator behavior has not been changed, so as long as opset_imports=[onnx.helper.make_opsetid("", 12)], is present, the test should pass just like it did before. I did not reintroduce this test to opset18 because storage_order is being removed.
There was a problem hiding this comment.
No I don't think z is wrong in the old opset tests where storage_order is set.
Only when storage_order is removed does z become incorrect - I just didn't notice the test was being removed. Removing it is a good fix for this :)
There was a problem hiding this comment.
My question was motivated by the adapter, rather than this test-case. If I understand correctly, simply omitting the storage_order attribute is not correct for converting a model with the old op to the new op ... do we need to insert some transpose to get the desired behavior of the original op (when the attribute is used)?
There was a problem hiding this comment.
In other words: it looks like the storage_order is a meaningful attribute ... it is used to convert the n-tuple of indices into a single index. It makes sense for MaxPool, since the value comes from a single index (ignoring duplicate max values), unlike the other pooling operations, and it is not relevant for other pooling ops, since they don't have such an index output. So, I wonder whether we should just the op be as is, instead of removing the attribute? (I agree that this is probably irrelevant for inference, since it is likely used for backprop in training, but we can't assume what users are using it for.)
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
…tors Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
…tors Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
|
After an internal discussion it has been decided that |
Signed-off-by: p-wysocki przemyslaw.wysocki@intel.com
Description
Add
MaxPool-18, which no longer hasstorage_orderattribute.Motivation and Context
Notes
onnx/backend/test/case/node/maxpool.pyhas been moved as an explicit MaxPool-12 test. I believe the way we should go about testing previous versions of operators needs to be clarified (related to [Feature request] Reduce potential clutter in "Examples" section in operator docs #4499).A related PR concerns adding
ceil_modeattribute to theLpPooloperator: Add LpPool-18 - addceil_modeanddilationsattributes #4534.