Skip to content

Abs layer support #6475

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 20, 2021
Merged

Abs layer support #6475

merged 4 commits into from
Jul 20, 2021

Conversation

DariaMityagina
Copy link
Contributor

No description provided.

@DariaMityagina DariaMityagina requested a review from a team July 1, 2021 01:17
@DariaMityagina DariaMityagina requested a review from a team as a code owner July 1, 2021 01:17
@DariaMityagina DariaMityagina requested a review from a team July 1, 2021 01:17
@openvino-pushbot openvino-pushbot added category: IE Tests OpenVINO Test: plugins and common category: VPU category: Core OpenVINO Core (aka ngraph) labels Jul 1, 2021
ilyachur
ilyachur reviewed Jul 1, 2021
View reviewed changes
inference-engine/src/readers/ir_reader_v7/ie_layer_parsers.cpp Outdated
@@ -42,7 +42,8 @@ CNNLayer::Ptr ActivationLayerCreator::CreateLayer(pugi::xml_node& node, LayerPar
{"elu", std::make_shared<LayerCreator<CNNLayer>>("ELU")},
{"sigmoid", std::make_shared<LayerCreator<CNNLayer>>("Sigmoid")},
{"tanh", std::make_shared<LayerCreator<CNNLayer>>("TanH")},
{"not", std::make_shared<LayerCreator<CNNLayer>>("LogicalNot")}
{"not", std::make_shared<LayerCreator<CNNLayer>>("LogicalNot")},
{"abs", std::make_shared<LayerCreator<CNNLayer>>("Abs")}
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 you use v7 parser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, removed.

ilyachur
ilyachur reviewed Jul 1, 2021
View reviewed changes
inference-engine/src/legacy_api/include/legacy/ie_layers.h
@@ -1025,7 +1025,8 @@ class INFERENCE_ENGINE_INTERNAL_CNNLAYER_CLASS(EltwiseLayer): public CNNLayer {
Logical_OR,
Logical_XOR,
Logical_NOT,
Mean
Mean,
Abs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you don't need to add Abs layer to Eltwise enum. Because in addSpecificCreator you create basic CNNLayer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

guess I need it in parser:
void FrontEnd::parseAbs(const Model &model, const ie::CNNLayerPtr &layer, const DataVector &inputs, const DataVector &outputs) const {
LayerParams params = {layer->name, "Eltwise", layer->precision};
auto res = std::make_shared<InferenceEngine::EltwiseLayer>(params);
res->_operation = InferenceEngine::EltwiseLayer::Abs;

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like after these changes we can get two behavior:

  • Convert ngraph to ICNNNetwork creates CNNLayer for Abs layer
  • Myriad frontend creates EltwiseLayer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed changes in addSpecificCreator.
So it is now processed in Myriad frontend.

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern was that I don't want to have two behavior.

Looks like after your changes all plugins which work with legacy representation will work with Abs layer as with CNNLayer and only VPU will interpret it as EltwiseLayer. Or am I wrong?
Could you align VPU behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right.
But the try to align VPU behavior to the other plugins leaded to failures in gna plugin. And processing Abs layer a bit differently on VPU solved the issue. I think this is the minimum amount of code to support the operation, which does not require changes in other plugins.
But if it's critical, I'll take another look at what I can do about it.
So, is it critical?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not critical because it is a legacy part and some plugins already migrated to new API.
But it is strange that changes in VPU plugin affect GNA 😄

Copy link
Contributor Author

@DariaMityagina DariaMityagina Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, then I'll leave this part as it is since it works.
Do you have any other comments?

inference-engine/src/legacy_api/src/convert_function_to_cnn_network.cpp Outdated
@@ -459,6 +459,8 @@ InferenceEngine::details::CNNLayerCreator::CNNLayerCreator(const std::shared_ptr
res->params["operation"] = "max";
} else if (node->description() == "Minimum") {
res->params["operation"] = "min";
} else if (node->description() == "Abs") {
Copy link
Contributor

Choose a reason for hiding this comment

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

What specific creator do you use? this creator or creator for Abs operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use creator for Abs operation
But it seems to break something for gna plugin
Trying to fix the failures

@DariaMityagina
Copy link
Contributor Author

@ilyachur could you please look once again?

@jgespino jgespino mentioned this pull request Jul 1, 2021
Closed
3 tasks
@jgespino jgespino linked an issue Jul 1, 2021 that may be closed by this pull request
Abs for PyTorch via ONNX not supported #6002
Closed
3 tasks
@DariaMityagina DariaMityagina requested a review from a team July 1, 2021 21:59
ilyachur
ilyachur reviewed Jul 2, 2021
View reviewed changes
inference-engine/src/legacy_api/src/ie_layer_validators.cpp Outdated
@@ -449,6 +449,8 @@ void EltwiseValidator::parseParams(CNNLayer* layer) {
casted->_operation = EltwiseLayer::Pow;
} else if (op == "mean") {
casted->_operation = EltwiseLayer::Mean;
} else if (op == "abs") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see any changes in the convert_function_to_cnn_network. Looks like the conversion won't create Eltwise operation for Abs layer.

We always create CNNLayer for Abs in legacy representation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Thanks.
Removed abs from here

@DariaMityagina DariaMityagina requested a review from ilyachur July 3, 2021 10:15
ilyachur
ilyachur approved these changes Jul 8, 2021
View reviewed changes
Copy link
Contributor

@ilyachur ilyachur left a comment

Choose a reason for hiding this comment

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

Common part LGTM

@DariaMityagina
Copy link
Contributor Author

@AntonDudchenko could you please take a look?

@AntonDudchenko
Copy link
Contributor

Changes from OV side LGTM

@DariaMityagina DariaMityagina force-pushed the icv/abs_op_support branch 2 times, most recently from f491792 to 9404523 Compare July 16, 2021 01:33
@DariaMityagina DariaMityagina merged commit b0ebea1 into openvinotoolkit:master Jul 20, 2021
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
@DariaMityagina
Abs layer support (openvinotoolkit#6475)
dc1f6c7 
* [MyriadX] Abs op support
andrei-cv pushed a commit to andrei-cv/openvino that referenced this pull request Aug 30, 2021
@DariaMityagina @andrei-cv
Abs layer support (openvinotoolkit#6475)
0ae2bf5 
* [MyriadX] Abs op support
akuporos pushed a commit to akuporos/openvino that referenced this pull request Sep 29, 2021
@DariaMityagina @akuporos
Abs layer support (openvinotoolkit#6475)
023516c 
* [MyriadX] Abs op support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilyachur ilyachur ilyachur approved these changes

@AntonDudchenko AntonDudchenko AntonDudchenko approved these changes

Assignees

@ilyachur ilyachur

Labels
category: Core OpenVINO Core (aka ngraph) category: IE Tests OpenVINO Test: plugins and common
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Abs for PyTorch via ONNX not supported
4 participants
@DariaMityagina @AntonDudchenko @ilyachur @openvino-pushbot