[GoodFirstIssue][Core] Created AvgPoolBase #23483

Vladislav-Denisov · 2024-03-15T12:40:22Z

Details:

Created AvgPoolBase class with the common functionality from AvgPool-1 and AvgPool-14

Tickets:

[Good First Issue]: Simplify AvgPool-14 and AvgPool-1 by implementing AvgPoolBase #23465

Vladislav-Denisov · 2024-03-15T12:48:08Z

src/core/include/openvino/op/util/avg_pool_base.hpp

+                const PadType& auto_pad = op::PadType::EXPLICIT);
+
+    void validate_and_infer_types() override;
+    bool visit_attributes(AttributeVisitor& visitor) override;


I suggest adding a similar method to the MaxPoolBase class with common attributes for v1, v8 and v14, as I did for AvgPoolBase

For example for MaxPool-8:

bool MaxPool::visit_attributes(AttributeVisitor& visitor) { OV_OP_SCOPE(v8_MaxPool_visit_attributes); visitor.on_attribute("index_element_type", m_index_element_type); visitor.on_attribute("axis", m_axis); return util::MaxPoolBase::visit_attributes(visitor); }

Good idea, let's have it in a separate PR.

Let's also do it in a separate Good First Issue. @Vladislav-Denisov would you like to create one or do you prefer I'd do it?

@p-wysocki Do you mean to create new GoodFirstIssue or only new PR?

I would be glad to create new PR to update MaxPoolBase implementation 😃

PR - certainly, Good First Issue - if you feel like it you can create one, if not, I'll do it, just let me know. :)

I can create both a PR and a GoodFirstIssue.
Thanks!

Vladislav-Denisov · 2024-03-15T12:49:23Z

@p-wysocki Hi! Could you review, please?

rkazants · 2024-03-15T13:07:09Z

src/core/include/openvino/op/util/avg_pool_base.hpp

+    void set_rounding_type(op::RoundingType rounding_type) {
+        m_rounding_type = rounding_type;
+    }


@p-wysocki, what if developer set torch rounding type to v1 version of AvgPool. Do we have check for this?
Do we really need this method for setting rounding type and what use cases? I think - not. Other setters are also questionable.

This setter can be required form GPU plugin as there is pattern to create op, set required attributes call shape infer (there is no node validation).
This attribute is important to be possible to set it.
The rounding mode is validated and torch mode should throw error for versions which are not supported. @p-wysocki could you confirm?

That is correct, it's validated here:

openvino/src/core/shape_inference/include/pooling_shape_inference_util.hpp

Lines 70 to 73 in d5b81f1

if (!has_torch_ceil_mode<TOp>()) {

const auto is_ceil_torch = op->get_rounding_type() == RoundingType::CEIL_TORCH;

NODE_VALIDATION_CHECK(op, !is_ceil_torch, "Rounding CEIL_TORCH is not supported.");

}

src/core/include/openvino/op/util/avg_pool_base.hpp

t-jankowski · 2024-03-18T08:16:36Z

src/core/include/openvino/op/util/avg_pool_base.hpp

+                const PadType& auto_pad = op::PadType::EXPLICIT);
+
+    void validate_and_infer_types() override;
+    bool visit_attributes(AttributeVisitor& visitor) override;


Good idea, let's have it in a separate PR.

src/core/src/op/util/avg_pool_base.cpp

p-wysocki

Thank you for your contribution!

LGTM, I have no other concerns than the ones from @praasz and @t-jankowski. Please request review once again when they're addressed. :)

p-wysocki · 2024-03-18T13:48:41Z

src/core/include/openvino/op/util/avg_pool_base.hpp

+                const PadType& auto_pad = op::PadType::EXPLICIT);
+
+    void validate_and_infer_types() override;
+    bool visit_attributes(AttributeVisitor& visitor) override;


Let's also do it in a separate Good First Issue. @Vladislav-Denisov would you like to create one or do you prefer I'd do it?

p-wysocki · 2024-03-18T13:54:35Z

src/core/include/openvino/op/util/avg_pool_base.hpp

+    void set_rounding_type(op::RoundingType rounding_type) {
+        m_rounding_type = rounding_type;
+    }


That is correct, it's validated here:

openvino/src/core/shape_inference/include/pooling_shape_inference_util.hpp

Lines 70 to 73 in d5b81f1

if (!has_torch_ceil_mode<TOp>()) {

const auto is_ceil_torch = op->get_rounding_type() == RoundingType::CEIL_TORCH;

NODE_VALIDATION_CHECK(op, !is_ceil_torch, "Rounding CEIL_TORCH is not supported.");

}

…tion constructor params.

…av-Denisov/openvino into feature/core/avgPoolBase

p-wysocki

LGTM, thank you for your contribution :)

p-wysocki · 2024-03-20T10:37:25Z

src/core/include/openvino/op/util/avg_pool_base.hpp

+                const PadType& auto_pad = op::PadType::EXPLICIT);
+
+    void validate_and_infer_types() override;
+    bool visit_attributes(AttributeVisitor& visitor) override;


PR - certainly, Good First Issue - if you feel like it you can create one, if not, I'll do it, just let me know. :)

p-wysocki · 2024-03-20T10:37:51Z

build_jenkins

mlukasze · 2024-03-20T10:41:00Z

please correct formatting to pass clang tests :)

p-wysocki · 2024-03-20T10:49:48Z

build_jenkins

src/core/src/op/avg_pool.cpp

praasz · 2024-03-22T11:04:30Z

build_jenkins

mlukasze · 2024-03-27T07:43:22Z

your first PR for OV has been merged @Vladislav-Deniso, congrats and thank you for support!

### Details: - *Created `AvgPoolBase` class with the common functionality from `AvgPool-1` and `AvgPool-14`* ### Tickets: - *openvinotoolkit#23465

[Core] Created AvgPoolBase

99ee282

Vladislav-Denisov requested a review from a team as a code owner March 15, 2024 12:40

github-actions bot added category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings labels Mar 15, 2024

Vladislav-Denisov commented Mar 15, 2024

View reviewed changes

rkazants added the ExternalPR External contributor label Mar 15, 2024

rkazants linked an issue Mar 15, 2024 that may be closed by this pull request

[Good First Issue]: Simplify AvgPool-14 and AvgPool-1 by implementing AvgPoolBase #23465

Closed

mlukasze requested review from t-jankowski and praasz March 15, 2024 12:57

rkazants reviewed Mar 15, 2024

View reviewed changes

praasz reviewed Mar 15, 2024

View reviewed changes

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

[Core] Moved impl from hpp to cpp

f9b2923

t-jankowski requested changes Mar 18, 2024

View reviewed changes

p-wysocki reviewed Mar 18, 2024

View reviewed changes

Vladislav-Denisov added 4 commits March 18, 2024 19:51

[Core] Changed nesting style for namespace and approach of initializa…

62cca0e

…tion constructor params.

[Core] Created AvgPoolBase

58bb366

[Core] Moved impl from hpp to cpp

8c8884a

[Core] Changed nesting style for namespace and approach of initializa…

0fa3244

…tion constructor params.

Vladislav-Denisov force-pushed the feature/core/avgPoolBase branch from 62cca0e to 0fa3244 Compare March 18, 2024 15:55

Merge branch 'feature/core/avgPoolBase' of https://github.com/Vladisl…

373844f

…av-Denisov/openvino into feature/core/avgPoolBase

Vladislav-Denisov requested review from p-wysocki, t-jankowski and praasz March 18, 2024 16:02

p-wysocki approved these changes Mar 20, 2024

View reviewed changes

Fix code-style

b27fb40

praasz requested changes Mar 21, 2024

View reviewed changes

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

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

src/core/src/op/avg_pool.cpp Show resolved Hide resolved

t-jankowski approved these changes Mar 21, 2024

View reviewed changes

p-wysocki mentioned this pull request Mar 21, 2024

[CPU] Add MaxPool-14 and AvgPool-14 #23582

Merged

praasz self-assigned this Mar 22, 2024

Restored passing parameter PadType via const ref

4080a71

praasz approved these changes Mar 22, 2024

View reviewed changes

praasz added this pull request to the merge queue Mar 26, 2024

akladiev removed this pull request from the merge queue due to the queue being cleared Mar 26, 2024

mlukasze requested a review from rkazants March 27, 2024 05:47

mlukasze added this pull request to the merge queue Mar 27, 2024

Merged via the queue into openvinotoolkit:master with commit 91d3d8a Mar 27, 2024
109 checks passed

mlukasze added this to the 2024.1 milestone Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GoodFirstIssue][Core] Created AvgPoolBase #23483

[GoodFirstIssue][Core] Created AvgPoolBase #23483

Vladislav-Denisov commented Mar 15, 2024

Vladislav-Denisov Mar 15, 2024

t-jankowski Mar 18, 2024

p-wysocki Mar 18, 2024

Vladislav-Denisov Mar 18, 2024

p-wysocki Mar 20, 2024

Vladislav-Denisov Mar 20, 2024

Vladislav-Denisov commented Mar 15, 2024

rkazants Mar 15, 2024 •

edited

Loading

praasz Mar 15, 2024

p-wysocki Mar 18, 2024

t-jankowski Mar 18, 2024

p-wysocki left a comment

p-wysocki Mar 18, 2024

p-wysocki Mar 18, 2024

p-wysocki left a comment

p-wysocki Mar 20, 2024

p-wysocki commented Mar 20, 2024

mlukasze commented Mar 20, 2024

p-wysocki commented Mar 20, 2024

praasz commented Mar 22, 2024

mlukasze commented Mar 27, 2024

	if (!has_torch_ceil_mode<TOp>()) {
	const auto is_ceil_torch = op->get_rounding_type() == RoundingType::CEIL_TORCH;
	NODE_VALIDATION_CHECK(op, !is_ceil_torch, "Rounding CEIL_TORCH is not supported.");
	}

[GoodFirstIssue][Core] Created AvgPoolBase #23483

[GoodFirstIssue][Core] Created AvgPoolBase #23483

Conversation

Vladislav-Denisov commented Mar 15, 2024

Details:

Tickets:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vladislav-Denisov commented Mar 15, 2024

rkazants Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-wysocki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-wysocki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-wysocki commented Mar 20, 2024

mlukasze commented Mar 20, 2024

p-wysocki commented Mar 20, 2024

praasz commented Mar 22, 2024

mlukasze commented Mar 27, 2024

rkazants Mar 15, 2024 •

edited

Loading