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

[VPU] Batch support for interpolate #5520

Merged
merged 9 commits into from
May 21, 2021

Conversation

DariaMityagina
Copy link
Contributor

@DariaMityagina DariaMityagina commented May 5, 2021

Details:

  • Interpolate batch support is required for the mask_rcnn_inception_v2_coco model to work

Tickets:

  • 51846

@DariaMityagina DariaMityagina requested review from a team May 5, 2021 17:24
@DariaMityagina DariaMityagina requested a review from a team as a code owner May 5, 2021 17:24
@openvino-pushbot openvino-pushbot added category: inference OpenVINO Runtime library - Inference category: IE Tests OpenVINO Test: plugins and common category: VPU labels May 5, 2021
const auto paramIsSupported = ic == oc && in == 1 && on == 1 && isPadZeros(padsBegin) && isPadZeros(padsEnd);
VPU_THROW_UNLESS(paramIsSupported, "Current Interpolate does not support paddings, batches, and resize by channels");
const auto paramIsSupported = ic == oc && isPadZeros(padsBegin) && isPadZeros(padsEnd);
VPU_THROW_UNLESS(paramIsSupported, "Current Interpolate does not support paddings, and resize by channels");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's split this message into 2 separate messages related to padding and channels accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{1, 8, 36, 36},
{1, 8, 35, 35},
{1, 8, 6, 6},
{3, 8, 38, 38},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not set batches for all test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

InferenceEngine::SizeVector, // Input shapes
InferenceEngine::SizeVector, // Target shapes
LayerTestsUtils::TargetDevice, // Device name
std::map<std::string, std::string> // Additional network configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is such a typedef somewhere

Copy link
Contributor Author

@DariaMityagina DariaMityagina May 7, 2021

Choose a reason for hiding this comment

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

Not in common files

@@ -50,4 +63,13 @@ class InterpolateLayerTest : public testing::WithParamInterface<InterpolateLayer
void SetUp() override;
};

class InterpolateLayerTestWithConfig : public testing::WithParamInterface<InterpolateLayerTestParamsWithConfig>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we'll leave only one test class and change instances in all plugins?
I don't like the current solution as far it causes a lot of ridiculous copy-pasta

Copy link
Contributor Author

@DariaMityagina DariaMityagina May 6, 2021

Choose a reason for hiding this comment

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

I did it before last commit.
But this will also require changes to the tests in the other plugins. Is is okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not? But you will have to use product-config repo to trigger a build with corresponding branches

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted changes with lots of copypasta

@DariaMityagina
Copy link
Contributor Author

@Maxim-Doronin @AntonDudchenko could you please approve this request if everything is okay?

@DariaMityagina DariaMityagina force-pushed the icv/batch_support branch 2 times, most recently from ff1c6ad to 45bceab Compare May 19, 2021 15:41
@azhogov
Copy link

azhogov commented May 21, 2021

@azhogov azhogov merged commit 5ea8437 into openvinotoolkit:master May 21, 2021
yekruglov pushed a commit to yekruglov/openvino that referenced this pull request Jun 7, 2021
* batch support for interpolate

* hash update

* compilation errors solved

* changes in tests, so not to break other plugins

* changes in tests, so not to break other plugins

* Revert "changes in tests, so not to break other plugins"

This reverts commit 67e5c1e.

* Revert "changes in tests, so not to break other plugins"

This reverts commit f6a537d.

* review corrections

* Firmware update
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
* batch support for interpolate

* hash update

* compilation errors solved

* changes in tests, so not to break other plugins

* changes in tests, so not to break other plugins

* Revert "changes in tests, so not to break other plugins"

This reverts commit 67e5c1e.

* Revert "changes in tests, so not to break other plugins"

This reverts commit f6a537d.

* review corrections

* Firmware update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: IE Tests OpenVINO Test: plugins and common category: inference OpenVINO Runtime library - Inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants