Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LpPool-18 - add ceil_mode and dilations attributes #4534

Merged
merged 11 commits into from
Nov 22, 2022

Conversation

p-wysocki
Copy link
Contributor

@p-wysocki p-wysocki commented Sep 22, 2022

Signed-off-by: p-wysocki przemyslaw.wysocki@intel.com

Description

Add LpPool-18, which now has ceil_mode and dilations attributes. Deprecate auto_pad, like it is done in the case of AveragePool and MaxPool.

Motivation and Context

Notes

  • LpPoolOpSchemaGenerator uses convPoolShapeInference, which already accounts for ceil_mode attribute. As far as I understand, it makes adding the attribute to ceil_mode trivial, as it is done in this PR. The same goes for the dilations attribute.
  • The tests for LpPool are non-existent (there is not such a file as onnx/onnx/backend/test/case/node/lppool.py). Is it intentional? Pooling operators seem to have a lot of common code, so maybe LpPool is considered to be covered by tests for MaxPool and AveragePool. I am not sure if it is intended or an overlook.
  • The issue touches on the subject of alignment of Pooling operators. Their attributes are as follows:
    image
    A related PR concerns removing storage_order attribute from the MaxPool operator: Add MaxPool-18 - remove storage_order attribute #4533.

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
@kraiskil
Copy link

I don't see why explicit LpPool tests would be left out intentionally. After all, the differnt pools calculate different things.

In my experience, when writing a backend, such coverage tests are very valuable, partly as a safety net but also as additional documentation / clarification. I can not see any big cost in adding such tests, except of course for the initial effort of writing them.

@gramalingam gramalingam added the operator Issues related to ONNX operators label Sep 27, 2022
@gramalingam
Copy link
Contributor

Agree with @kraiskil ... adding the test-cases would be useful.

@gramalingam
Copy link
Contributor

Why not add the dilations attribute as well?

@gramalingam gramalingam added this to the 1.13 milestone Oct 13, 2022
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
@p-wysocki
Copy link
Contributor Author

@kraiskil @gramalingam I'm working on both adding tests and adding dilations attribute.

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>
onnx/backend/base.py Outdated Show resolved Hide resolved
@p-wysocki p-wysocki changed the title Add LpPool-18 - add ceil_mode attribute Add LpPool-18 - add ceil_mode and dilations attributes Nov 9, 2022
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
@gramalingam gramalingam enabled auto-merge (squash) November 21, 2022 16:57
@gramalingam gramalingam merged commit bad0697 into onnx:main Nov 22, 2022
justinchuby pushed a commit to justinchuby/onnx that referenced this pull request Jan 27, 2023
* Add LpPool-18

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

* Rework adapter

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

* Minor changes

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

* Add dilations

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

* Add shape inference tests for dilations

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Co-authored-by: G. Ramalingam <grama@microsoft.com>
broune pushed a commit to broune/onnx that referenced this pull request May 6, 2023
* Add LpPool-18

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

* Rework adapter

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

* Minor changes

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

* Add dilations

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

* Add shape inference tests for dilations

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>

Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Co-authored-by: G. Ramalingam <grama@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operator Issues related to ONNX operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants