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 squeeze v15 op spec #26989

Merged
merged 14 commits into from
Nov 4, 2024
Merged

Conversation

barnasm1
Copy link
Contributor

Details:

  • Add squeeze v15 op spec

Tickets:

@barnasm1 barnasm1 added the category: Core OpenVINO Core (aka ngraph) label Oct 10, 2024
@barnasm1 barnasm1 self-assigned this Oct 10, 2024
@barnasm1 barnasm1 requested a review from a team as a code owner October 10, 2024 08:34
@barnasm1 barnasm1 requested review from tadamczx and removed request for a team October 10, 2024 08:34
@github-actions github-actions bot added category: docs OpenVINO documentation and removed category: Core OpenVINO Core (aka ngraph) labels Oct 10, 2024

* *T_INT*: any supported integer type.

**Example**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add attribute value to example IR. Consider adding additional examples that will show differences between attribute values and maybe examples of dynamic rank or ignored non-1 dimensions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@mmikolajcz
Copy link
Contributor

PR that adds opset15 to documentation was merged, please update following files to link added specification:

  • docs/articles_en/documentation/openvino-ir-format/operation-sets/available-opsets/opset15.rst
  • docs/articles_en/documentation/openvino-ir-format/operation-sets/operation-specs.rst

Copy link
Contributor

@mitruska mitruska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Note" part seems to describe only the old default behavior, without considering the new allow_axis_skip attribute.


.. note::

- If index of the dimension to squeeze is provided as a constant input and it points to a dynamic dimension that might be `1`, then the dimension is considered as squeezable. Therefore the rank of the output shape will be reduced, but not dynamic.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- If index of the dimension to squeeze is provided as a constant input and it points to a dynamic dimension that might be `1`, then the dimension is considered as squeezable. Therefore the rank of the output shape will be reduced, but not dynamic.
- If index of the dimension to squeeze is provided as a constant input and it points to a dynamic dimension that might be `1`, and the *allow_axis_skip* attribute is ``false``, then the dimension is considered as squeezable. Therefore the rank of the output shape will be reduced, but not dynamic. If dynamic rank is expected for such case, *allow_axis_skip* attribute need to be set to ``true``.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

**Attributes**:

* *allow_axis_skip*

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect exception raised when allow_axis_skip=False and given axis corresponds to dimension that is not squeezable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be implemented to throw, but the intention is to keep the v0::Squeeze compatible behavior when allow_axis_skip=False.
For model enablement the v0::Squeeze has been already updated to be not so strict for static dimensions, but it was not possible to update the all dynamic cases and keep backward compatibility. Especially previous Squeeze logic assumes that dynamic dim with 1 in range like {-1} or {1-2} is squeezable, and produces static rank.
While the recent request to align with torch is to produce dynamic rank for such case.
In fact allow_axis_skip=False can be understood like assume_dynamic_dim_is_squeezable=True.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks that this mode is also helpful for TF Squeeze op (https://www.tensorflow.org/api_docs/python/tf/raw_ops/Squeeze) when axis = []. For such case, I can compute axes for our Squeeze using ShapeOf and Range ops and set this mode allow_axis_skip=False. This way it will squeeze all dims that are possible to squeeze. So we can squeeze dimension during both conversion/loading and inference, right?

@@ -0,0 +1,174 @@
Squeeze
=======

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we require such version? Is there similar operation in frameworks like PyTorch, TF, ONNX? Can we refer a link to such operation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed for alignment with torch, more details can be found in the internal ticket: 143585.

@barnasm1 barnasm1 added this pull request to the merge queue Nov 4, 2024
@barnasm1 barnasm1 removed this pull request from the merge queue due to a manual request Nov 4, 2024
@barnasm1 barnasm1 added this pull request to the merge queue Nov 4, 2024
Merged via the queue into openvinotoolkit:master with commit 4d8ff86 Nov 4, 2024
132 checks passed
@barnasm1 barnasm1 deleted the squeeze_v15_spec branch November 4, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: docs OpenVINO documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants