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

[GNA] Support for cascade concat with non functional layers #598

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

admitriev-gna
Copy link
Contributor

Support for cascade concat with non functional layers between concats.
Added tests for cascade concat

@admitriev-gna admitriev-gna requested review from esmirno, dorloff, pavel-rodionov and a team May 26, 2020 12:24
@admitriev-gna admitriev-gna self-assigned this May 26, 2020
@pavel-rodionov pavel-rodionov added the category: GNA OpenVINO GNA plugin label May 27, 2020
@admitriev-gna
Copy link
Contributor Author

@dorloff, @esmirno check please)

Copy link
Contributor

@pavel-rodionov pavel-rodionov left a comment

Choose a reason for hiding this comment

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

Looks good just some minor comments.

@@ -550,6 +550,23 @@ void GNAGraphCompiler::ConcatPrimitive(InferenceEngine::CNNLayerPtr layer) {
auto& concatLayerInfo = concat_connection.find(concatLayer->name)->second;
for (auto &&outLayer : concatLayer->outData.front()->getInputTo()) {
if ( LayerInfo(outLayer.second).isConcat() ) {
auto concatCadidate = outLayer.second;
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here

#include <memory>
#include "functional_test_utils/layer_test_utils.hpp"
#include "ngraph_functions/builders.hpp"
#include "ngraph_functions/utils/ngraph_helpers.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry but do you actually use something form ngraph_helpers.hpp here in this header?

#include <string>
#include <vector>
#include <memory>
#include <debug.h>
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 need this?

const_concat2->output(0)},
0);
auto const_mul = ngraph::builder::makeConstant(ngPrc, ngraph::Shape{1}, std::vector<float>{-1.0f});
auto mult = std::make_shared<ngraph::opset1::Multiply>(input[0], const_mul);
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency I can suggest your to use const_mul and mul or const_mult and mult

function = std::make_shared<ngraph::Function>(mult, input, "concat_reshape_reshape_concat_mul");
}

TEST_P(ConcatReshapeReshapeConcatMul, CompareWithRefs){
Copy link
Contributor

Choose a reason for hiding this comment

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

why we are not creating unit tests at all? Functional test to me only should happen if fight with accuracy. And event than it is questionable.

if (!CNNNetHasNextLayerSkipCertain(concatCadidate, 0, 0, isNonFunctional)) {
continue;
}
concatCadidate = CNNNetGetNextLayerSkipCertain(concatCadidate, 0, 0, isNonFunctional).first;
Copy link
Contributor

Choose a reason for hiding this comment

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

what if there will be non linear subgraph of reshapes? What functional layer will be ended up with?
Create a case concat->reshape->(out:0)affine, (out:1 )concat

@admitriev-gna admitriev-gna force-pushed the private/ad/permute branch 7 times, most recently from 59721ac to 2e75b62 Compare June 24, 2020 07:13
@admitriev-gna admitriev-gna force-pushed the private/ad/permute branch 4 times, most recently from 0932d40 to 3b5b328 Compare June 25, 2020 13:01
…oncats

[GNA] Support for cascade concat with non functional layers between concats

[GNA] Support for cascade concat with non functional layers between concats

fix discussion

medn

test

delete commet

Rework test

minor fix

last fix

Test Align filter

fix discussion
@esmirno esmirno merged commit 861bcc2 into openvinotoolkit:master Jul 30, 2020
RomanZm pushed a commit to RomanZm/openvino that referenced this pull request Aug 28, 2020
…oncats (openvinotoolkit#598)

[GNA] Support for cascade concat with non functional layers between concats
tadamowicz pushed a commit to tadamowicz/openvino that referenced this pull request Aug 30, 2023
…oncats (openvinotoolkit#598)

[GNA] Support for cascade concat with non functional layers between concats
redradist pushed a commit to redradist/openvino that referenced this pull request Oct 6, 2023
…it#598)

* Modify OPENVINO_OP using, caused by deprecated version

Signed-off-by: xuejun <[email protected]>

* Fix review comments

Signed-off-by: xuejun <[email protected]>

---------

Signed-off-by: xuejun <[email protected]>
Co-authored-by: Ilya Lavrenov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: GNA OpenVINO GNA plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants