Comparison of InputDescription and OutputDescription in SubGraph by elszkowski · Pull Request #4123 · openvinotoolkit/openvino

elszkowski · 2021-02-01T15:00:21Z

Review Refactoring FunctionsComparator - extract node comparison part #4175 before this.

Changes:

add comparison between ngraph::Functions [inputs, parameters] and [outputs, result] comparison -> subgraph::compare_io
add storage for ngraph::Function
drop SpecialBodyPorts from storage

Ticket 47693

inference-engine/tests/functional/inference_engine/transformations/compare_functions_test.cpp

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp

ilyachur

This PR contains a lot of TODO marks. Can we implement these TODOs in this PR?

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp

inference-engine/tests/functional/inference_engine/transformations/compare_functions_test.cpp

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp

itikhono · 2021-02-17T07:47:11Z

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp

+
+        if (description->get_type_info() == SubGraphOp::ConcatOutputDescription::type_info) {
+            ///
+            /// TODO shape calculation - required decent formula to check size from Slice


we need to know the count of iterations (m_iterations member in Loop/TensorIterator) to calculate the output shape correctly.
I guess this check shouldn't be a part of this Checker, we already have similar checks in validate_and_infer_type function of Loop/TI. @ilyachur @GlebKazantaev what do you think?

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp

sdurawa · 2021-02-23T10:19:54Z

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp

+        }
+    }
+
+    void verify_else(const std::string& name, ngraph::ValueAccessor<void>& adapter) {


function name is confusing, it does not provide information of its purpose.

It's not best name but for now I can't find any better.
verify_others will be fine?

sdurawa · 2021-02-23T10:52:27Z

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp

+        } else if (lhs->get_type_info() == SubGraphOp::InvariantInputDescription::type_info) {
+            return true;  // noting extra to check
+        }
+


missing else{} statement for the exception handling, the same issue appears in other matching and equal methods.

I'm not sure if extra else is required. Exception will be throw with or without else.

sdurawa · 2021-02-23T11:47:21Z

ngraph/core/src/op/tensor_iterator.cpp

-            m_num_iterations = ((std::abs(concat->m_end - concat->m_start)) / concat->m_part_size);
-        }
-    }
+    set_num_iteratrions_if_no_slice_inputs(); // TODO can be move to validate_and_infer_types


why adding TODO instead moving it to validate_and_infer_types?

Because I want to discus this question in this review.

jdanieck · 2021-02-25T09:20:48Z

@ilyachur do you plan to merge this before CF or after ? I mean, can we expect new bugs discovered just before CF or not ;)

ilyachur · 2021-02-25T10:31:55Z

@ilyachur do you plan to merge this before CF or after ? I mean, can we expect new bugs discovered just before CF or not ;)

I think it will be good to have the full picture. And if this PR will show new issues I think we should know about it.

sdurawa

LGTM, one thing which can improve readability of the PR are comments, at least descriptions for classes and methods (lots of them are not self explanatory, ie 'verify_others', 'compare').

elszkowski · 2021-02-25T13:07:58Z

LGTM, one thing which can improve readability of the PR are comments, at least descriptions for classes and methods (lots of them are not self explanatory, ie 'verify_others', 'compare').

https://youtu.be/ZsHMHukIlJY?t=617
:)

elszkowski · 2021-02-25T13:09:05Z

@ilyachur, all green now.

elszkowski requested a review from itikhono February 1, 2021 15:00

itikhono reviewed Feb 2, 2021

View reviewed changes

inference-engine/tests/functional/inference_engine/transformations/compare_functions_test.cpp Outdated Show resolved Hide resolved

itikhono reviewed Feb 2, 2021

View reviewed changes

inference-engine/tests/functional/inference_engine/transformations/compare_functions_test.cpp Show resolved Hide resolved

itikhono reviewed Feb 2, 2021

View reviewed changes

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp Show resolved Hide resolved

elszkowski added the DO NOT MERGE label Feb 3, 2021

jdanieck mentioned this pull request Feb 10, 2021

Refactoring FunctionsComparator - extract node comparison part #4175

Merged

elszkowski marked this pull request as ready for review February 16, 2021 08:47

elszkowski requested review from a team, GlebKazantaev, ilyachur, itikhono and jdanieck February 16, 2021 08:47

ilyachur reviewed Feb 17, 2021

View reviewed changes

elszkowski commented Feb 17, 2021

View reviewed changes

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp Outdated Show resolved Hide resolved

itikhono reviewed Feb 17, 2021

View reviewed changes

elszkowski requested review from ilyachur and itikhono February 17, 2021 09:34

elszkowski commented Feb 19, 2021

View reviewed changes

inference-engine/tests/ie_test_utils/common_test_utils/ngraph_test_utils.cpp Outdated Show resolved Hide resolved

Patryk Elszkowski added 6 commits February 19, 2021 14:19

draft - looks good, need merge with other branch

1ee8263

apply suggestions from PR

932df05

apply suggestions from PR

4f46ce1

add unit test - NotValidInputOrOutput

48303b1

rephraze equal for constant

422eb1c

cleanup all TODOs

8394339

elszkowski requested a review from sdurawa February 22, 2021 11:28

Try to fix TI

8a49686

elszkowski removed the DO NOT MERGE label Feb 23, 2021

sdurawa reviewed Feb 23, 2021

View reviewed changes

Patryk Elszkowski added 2 commits February 24, 2021 09:28

try to satisfy PR comments

c65325c

Merge branch 'master' into compare_function-compare_subgraph

d567c49

jdanieck removed their request for review February 24, 2021 12:36

ilyachur approved these changes Feb 25, 2021

View reviewed changes

fix spelling

82e76b9

sdurawa approved these changes Feb 25, 2021

View reviewed changes

ilyachur merged commit af71274 into openvinotoolkit:master Feb 25, 2021

elszkowski deleted the compare_function-compare_subgraph branch February 26, 2021 06:41

Conversation

elszkowski commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilyachur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

itikhono Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sdurawa Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elszkowski Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdurawa Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elszkowski Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

sdurawa Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elszkowski Feb 23, 2021

Choose a reason for hiding this comment

Uh oh!

jdanieck commented Feb 25, 2021

Uh oh!

ilyachur commented Feb 25, 2021

Uh oh!

sdurawa left a comment

Choose a reason for hiding this comment

Uh oh!

elszkowski commented Feb 25, 2021

Uh oh!

elszkowski commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elszkowski commented Feb 1, 2021 •

edited

Loading

sdurawa Feb 23, 2021 •

edited

Loading

elszkowski Feb 23, 2021 •

edited

Loading

sdurawa Feb 23, 2021 •

edited

Loading

sdurawa Feb 23, 2021 •

edited

Loading

elszkowski commented Feb 25, 2021 •

edited

Loading