squeeze v15 implementation #26995

barnasm1 · 2024-10-10T12:06:21Z

Details:

Add v15::Squeeze class with support dynamic rank result based on v0::Squeeze

Tickets:

154023

mitruska

The changes should be covered by tests, as we have discussed please follow similar merged PR for example: #23754
The main reason for introducing new version of Squeeze op is the output shape inference logic update (not included in this PR so far).

mitruska · 2024-10-10T14:38:13Z

src/core/include/openvino/op/squeeze.hpp

+namespace v15 {
+class OPENVINO_API Squeeze : public v0::Squeeze {


Usage of the previous op version as a base class seems convenient, but may lead to some issues.
For example, in the transformations the wrap_type<v0::Squeeze> will match to v15::Squeeze as well, because of its base class.

Also validate_and_infer_types will be called for the v0::Squeeze constructor (and for v15::Squeeze if overwritten to call updated shape_infer).

openvino/src/core/src/op/squeeze.cpp

Lines 127 to 129 in be046ac

namespace v15 {

Squeeze::Squeeze(const Output<Node>& data, const Output<Node>& axes, const bool allow_axis_skip)

: v0::Squeeze({data, axes}),

Similar issue has been resolved for TopK (PR):

To follow current approach it would be better to create SqueezeBase class to share between v1::Squeeze and v15::Squeeze.

cc: @itikhono, @praasz

src/core/include/openvino/op/squeeze.hpp

src/core/shape_inference/include/squeeze_shape_inference.hpp

src/core/tests/type_prop/squeeze.cpp

src/plugins/intel_cpu/tests/unit/shape_inference_test/squeeze_shape_inference_test.cpp

src/core/tests/type_prop/squeeze.cpp

src/core/src/op/squeeze.cpp

mitruska · 2024-10-30T08:28:58Z

src/core/shape_inference/include/squeeze_shape_inference.hpp

@@ -59,7 +170,7 @@ std::vector<TRShape> shape_infer(const Squeeze* op,
                        return dim.compatible(1);
                    });
                if (has_squeezable_dim) {
-                    output_shape = PartialShape::dynamic(arg_rank.get_length() - 1);
+                    output_shape = PartialShape::dynamic({arg_rank.get_length() - 1, arg_rank.get_length()});


Could you explain what is the expected result for such PartialShape definition and point specific unit test for this change?
It looks like Rank object initialized with range Dimension (while rank can be only static or dynamic, ranges for Rank are not supported).

output_shape = PartialShape::dynamic({arg_rank.get_length() - 1, arg_rank.get_length()})

There are a few test for that particular case: 6450f72#diff-17ddb7d4bd361c05e41dd9a2e22a538ddf79520a78bdd0fef7373540fe31ffddR100
If needed I can change it to fully dynamic result instead of expected Rank range.

The defined range for the rank has no effect here.
Such instantiation always leads to fully dynamic dimension, the provided range for the Rank is ignored.
All of the tests with replaced PartialShape::dynamic({2, 3}) to PartialShape::dynamic() will pass.

openvino/src/core/src/partial_shape.cpp

Lines 174 to 176 in 961b891

ov::PartialShape ov::PartialShape::dynamic(Rank r) {

return PartialShape(r.is_static(),

std::vector<Dimension>(r.is_static() ? r.get_length() : 0, Dimension::dynamic()));

mitruska

The range defined for the Rank has no effect,
such instantiation always leads to fully dynamic dimension, the provided range for the Rank is ignored.
All of the implemented tests with expected PartialShape::dynamic({2, 3}) replaced to PartialShape::dynamic() will pass as well.

src/core/tests/type_prop/squeeze.cpp

mitruska · 2024-10-30T09:01:10Z

src/core/tests/type_prop/squeeze.cpp

+        EXPECT_EQ(squeeze->get_output_partial_shape(0), exp_shape.to_shape());
+    }
+    {
+        const auto squeeze = std::make_shared<op::v15::Squeeze>(param, axes_node);
+        EXPECT_EQ(squeeze->get_element_type(), element::f64);
+        EXPECT_EQ(squeeze->get_output_partial_shape(0), exp_shape.to_shape());


Is .to_shape() really needed while comparing with PartialShape?

mitruska · 2024-10-30T09:06:24Z

src/core/shape_inference/include/squeeze_shape_inference.hpp

@@ -59,7 +170,7 @@ std::vector<TRShape> shape_infer(const Squeeze* op,
                        return dim.compatible(1);
                    });
                if (has_squeezable_dim) {
-                    output_shape = PartialShape::dynamic(arg_rank.get_length() - 1);
+                    output_shape = PartialShape::dynamic({arg_rank.get_length() - 1, arg_rank.get_length()});


The defined range for the rank has no effect here.
Such instantiation always leads to fully dynamic dimension, the provided range for the Rank is ignored.
All of the tests with replaced PartialShape::dynamic({2, 3}) to PartialShape::dynamic() will pass.

openvino/src/core/src/partial_shape.cpp

Lines 174 to 176 in 961b891

ov::PartialShape ov::PartialShape::dynamic(Rank r) {

return PartialShape(r.is_static(),

std::vector<Dimension>(r.is_static() ? r.get_length() : 0, Dimension::dynamic()));

src/core/tests/type_prop/squeeze.cpp

### Details: - *Add Squeeze-15 downgrade transformation to Squeeze-0 for compatible attribute* - *...* ### Tickets: - *CVS-154027* ### PR requires [PR-26995](#26995) to be merged --------- Co-authored-by: Michal Lukaszewski <[email protected]>

### Details: - This PR includes commits from #26995 ### Tickets: - CVS-154024 --------- Signed-off-by: p-wysocki <[email protected]> Co-authored-by: Michal Barnas <[email protected]> Co-authored-by: Roman Kazantsev <[email protected]> Co-authored-by: Michal Lukaszewski <[email protected]>

squeeze v15 implementation

9c8f261

barnasm1 requested a review from a team as a code owner October 10, 2024 12:06

barnasm1 self-assigned this Oct 10, 2024

barnasm1 added the category: Core OpenVINO Core (aka ngraph) label Oct 10, 2024

github-actions bot added the category: CPP API OpenVINO CPP API bindings label Oct 10, 2024

use attribute name from spec

be046ac

mitruska requested changes Oct 10, 2024

View reviewed changes

barnasm1 added 3 commits October 11, 2024 14:15

Merge branch 'master' into squeeze_v15_class

4a27b4a

squeeze v15 no common bese class

8830412

copy shape_infer impl for tests

bf46e42

mlukasze requested a review from mitruska October 21, 2024 05:03

praasz reviewed Oct 21, 2024

View reviewed changes

src/core/include/openvino/op/squeeze.hpp Outdated Show resolved Hide resolved

barnasm1 added 5 commits October 22, 2024 11:30

add tests squeeze v15

bfd61ec

add visitor_single_attribute test

757ba2b

clang foramt

971cb46

Merge branch 'master' into squeeze_v15_class

c1942ec

turn off Wsuggest-override for typed test

0900e09

github-actions bot added the category: build OpenVINO cmake script / infra label Oct 22, 2024

barnasm1 added 8 commits October 22, 2024 13:55

use valid constructor

47792dd

turn off failing tests

c4fa858

prevent miss use squeeze shape_infer

1ad2ad1

clang format

96bc057

Merge branch 'master' into squeeze_v15_class

acd8bff

Merge branch 'master' into squeeze_v15_class

73dd231

add util namespace

6727564

Merge branch 'master' into squeeze_v15_class

693ca67

praasz reviewed Oct 24, 2024

View reviewed changes

barnasm1 added 3 commits October 24, 2024 12:51

Merge branch 'master' into squeeze_v15_class

bd12551

review suggestions for tests

28fd904

Merge branch 'master' into squeeze_v15_class

8875216

praasz reviewed Oct 29, 2024

View reviewed changes

mmikolajcz reviewed Oct 29, 2024

View reviewed changes

src/core/src/op/squeeze.cpp Outdated Show resolved Hide resolved

barnasm1 added 2 commits October 29, 2024 14:43

Merge branch 'master' into squeeze_v15_class

0f47b7a

review suggestions

2a1979b

mlukasze requested review from mmikolajcz and praasz October 30, 2024 05:49

Merge branch 'master' into squeeze_v15_class

9a220c7

barnasm1 enabled auto-merge October 30, 2024 07:45

praasz disabled auto-merge October 30, 2024 07:50

praasz approved these changes Oct 30, 2024

View reviewed changes

Merge branch 'master' into squeeze_v15_class

9d8b408

barnasm1 enabled auto-merge October 30, 2024 08:23

mmikolajcz approved these changes Oct 30, 2024

View reviewed changes

mitruska reviewed Oct 30, 2024

View reviewed changes

mitruska requested changes Oct 30, 2024

View reviewed changes

barnasm1 added 2 commits October 30, 2024 09:17

restore full dynamic rank result

051dad1

revert to_shape call

7c6878b

mitruska reviewed Oct 30, 2024

View reviewed changes

src/core/tests/type_prop/squeeze.cpp Show resolved Hide resolved

missing bracket

a7540e4

mitruska approved these changes Oct 30, 2024

View reviewed changes

barnasm1 added 4 commits October 30, 2024 12:57

Merge branch 'master' into squeeze_v15_class

11f0637

Merge branch 'master' into squeeze_v15_class

b6cac31

Merge branch 'master' into squeeze_v15_class

b76abc2

Merge branch 'master' into squeeze_v15_class

29ba4c3

barnasm1 added this pull request to the merge queue Oct 30, 2024

Merged via the queue into openvinotoolkit:master with commit 6e35049 Oct 30, 2024
166 checks passed

barnasm1 deleted the squeeze_v15_class branch October 30, 2024 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

squeeze v15 implementation #26995

squeeze v15 implementation #26995

barnasm1 commented Oct 10, 2024 •

edited

Loading

mitruska left a comment

mitruska Oct 10, 2024

barnasm1 Oct 29, 2024

mitruska Oct 30, 2024 •

edited

Loading

barnasm1 Oct 30, 2024

mitruska Oct 30, 2024

mitruska left a comment

mitruska Oct 30, 2024

mitruska Oct 30, 2024

		namespace v15 {
		class OPENVINO_API Squeeze : public v0::Squeeze {

	namespace v15 {
	Squeeze::Squeeze(const Output<Node>& data, const Output<Node>& axes, const bool allow_axis_skip)
	: v0::Squeeze({data, axes}),

	ov::PartialShape ov::PartialShape::dynamic(Rank r) {
	return PartialShape(r.is_static(),
	std::vector<Dimension>(r.is_static() ? r.get_length() : 0, Dimension::dynamic()));

squeeze v15 implementation #26995

squeeze v15 implementation #26995

Conversation

barnasm1 commented Oct 10, 2024 • edited Loading

Details:

Tickets:

mitruska left a comment

Choose a reason for hiding this comment

mitruska Oct 10, 2024

Choose a reason for hiding this comment

barnasm1 Oct 29, 2024

Choose a reason for hiding this comment

mitruska Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

barnasm1 Oct 30, 2024

Choose a reason for hiding this comment

mitruska Oct 30, 2024

Choose a reason for hiding this comment

mitruska left a comment

Choose a reason for hiding this comment

mitruska Oct 30, 2024

Choose a reason for hiding this comment

mitruska Oct 30, 2024

Choose a reason for hiding this comment

barnasm1 commented Oct 10, 2024 •

edited

Loading

mitruska Oct 30, 2024 •

edited

Loading