Fix experimental detectron do ref impl #10621

opoluektov-lohika · 2022-02-23T13:02:46Z

Fix ExperimentalDetectronDetectionOutput ngraph implementation according to documentation

tomdol

Overall I think the changes are ok but I'd like to know if there's any particular ticket related to this PR.
Also it seems that some more tests have failed because the behavior of this op has been changed.
AFAIK there are some translation models that contain this op - those should be validated on this branch before the PR is merged.

docs/template_plugin/backend/evaluates_map.cpp

docs/template_plugin/tests/functional/op_reference/experimental_detectron_detection_output.cpp

opoluektov-lohika · 2022-05-18T22:50:17Z

@tomdol at the very least CPU implementation and src/tests/functional/shared_test_classes/src/single_layer/experimental_detectron_detection_output.cpp test data should be adapted accordingly.

Please let me know if you would like me to go forward with those changes.

tomdol · 2022-05-19T06:28:32Z

Please let me know if you would like me to go forward with those changes.

I think me (and other folks) need some context where this PR comes from and how it's motivated. Do you have a ticket number where I could read some more about it?

opoluektov-lohika · 2022-05-19T11:33:27Z

Please let me know if you would like me to go forward with those changes.

I think me (and other folks) need some context where this PR comes from and how it's motivated. Do you have a ticket number where I could read some more about it?

The PR comes from my study of reference implementation of the operation (as GPU implementation of EDDO is assigned to me).

While comparing the code with the spec, and trying to make sense of test data (input and output), I noticed a few surprising things, and subsequently notified Intel team via email (Pavel Durandin was the contact point at that time), and later in the meeting it was decided that I will produce this PR.

I'm not aware of any ticket related to this PR.

tomdol

Looks good from my perspective but I've asked some more folks to review.

docs/template_plugin/backend/evaluates_map.cpp

…on test for readability

…lasses Due to bug in indexing classes, 0-th class was not considered.

…lip to image The resulting boxes must be clipped to the image according the op spec. Test case modified to cover the change in the implementation.

… implementation To check the output scores really belong to the corresponding classes.

…OIs count It is not the same as max_detections_from_image as was incorrectly assumed in the code. Test case modified to cover the change in the implementation.

... to match reference implementation

So that the previous fixes in the operation implementations are actually tested.

... accordingly to the changes in the reference implementation.

…mat checcker happy

usstq · 2022-08-19T07:57:52Z

src/plugins/intel_cpu/src/nodes/experimental_detectron_detection_output.cpp

@@ -310,7 +327,7 @@ void ExperimentalDetectronDetectionOutput::execute(dnnl::stream strm) {
    std::vector<int> detections_per_class(classes_num_, 0);
    int total_detections_num = 0;

-    for (int class_idx = 1; class_idx < classes_num_; ++class_idx) {
+    for (int class_idx = 0; class_idx < classes_num_; ++class_idx) {


Hi @tomdol @t-jankowski @opoluektov-lohika This change introduced a regression in e2e test (ticket: 87044), I guess because this change adds background classes to the outputs, is there any solid reason for this change ?

Hi @usstq, this change is to harmonize the ref implementation with the op specification

Unfortunately in the spec I hanen't found any tip

that the 0th class is always a background class

that the background class should always be removed

I guess this should be handled together with class_agnostic_box_regression attribute.
(But since this attr has been unhandled in the reference implementation, and since I hadn't understood the meaning of this attr at the time of the fix, after some discussion we decided to leave it as it is, unhandled.)

@usstq please drop me a line to my email (in the github profile), I'll provide you with some more context

The documentation of Detectron2 also confirms that in Detectron(not Detectron2) class 0 is assumed to be background and should be filtered :
https://detectron2.readthedocs.io/en/latest/notes/compatibility.html?highlight=background#compatibility-with-detectron-and-maskrcnn-benchmark

PR #12663 is submitted to fix this behavior

jane-intel · 2022-08-22T09:39:21Z

Spec is a word explanation of the reference implementation. As this PR introduced a regression -- I revert the change. Let's review it further including the question if we need to introduce new version of this operation due to changed output calculation logic.

@opoluektov-lohika what inspired you to perform these changes?

This reverts commit d872338.

opoluektov-lohika · 2022-08-22T13:25:34Z

@jane-intel I just sent you an email conversation that explains the rationale.

This reverts commit d872338.

* Revert "Fix experimental detectron do ref impl (#10621)" This reverts commit d872338. * Disabled Experimental Detectron per agreement with GPU team. Ticket to fix it: 90209

)" This reverts commit 4daee82.

…" (openvinotoolkit#12683) * Revert "Fix experimental detectron do ref impl (openvinotoolkit#10621)" This reverts commit d872338. * Disabled Experimental Detectron per agreement with GPU team. Ticket to fix it: 90209

…3009) * Revert "Fix experimental detectron do ref impl (#10621)" This reverts commit d872338. * Disabled Experimental Detectron per agreement with GPU team. Ticket to fix it: 90209

opoluektov-lohika requested review from a team February 23, 2022 13:02

openvino-pushbot added ExternalPR External contributor category: TEMPLATE OpenVINO Template plugin category: Core OpenVINO Core (aka ngraph) labels Feb 23, 2022

openvinotoolkit blocked opoluektov-lohika Feb 28, 2022

AnastasiaKazantaeva removed the ExternalPR External contributor label Mar 9, 2022

opoluektov-lohika force-pushed the fix-experimental-detectron-do-ref-impl branch from 3fb614f to a0d9d16 Compare May 17, 2022 23:01

opoluektov-lohika requested a review from a team as a code owner May 17, 2022 23:01

tomdol reviewed May 18, 2022

View reviewed changes

t-jankowski reviewed May 18, 2022

View reviewed changes

docs/template_plugin/backend/evaluates_map.cpp Outdated Show resolved Hide resolved

docs/template_plugin/tests/functional/op_reference/experimental_detectron_detection_output.cpp Outdated Show resolved Hide resolved

opoluektov-lohika force-pushed the fix-experimental-detectron-do-ref-impl branch from a0d9d16 to 26cdee3 Compare May 24, 2022 12:06

opoluektov-lohika requested review from a team as code owners May 24, 2022 12:06

opoluektov-lohika force-pushed the fix-experimental-detectron-do-ref-impl branch from 26cdee3 to 2c90fdc Compare May 24, 2022 18:52

wilson-seok added the ExternalPR External contributor label May 25, 2022

opoluektov-lohika force-pushed the fix-experimental-detectron-do-ref-impl branch 2 times, most recently from a22437d to 73ea06d Compare May 31, 2022 11:29

opoluektov-lohika mentioned this pull request May 31, 2022

[GPU] Implement ExperimentalDetectronDetectionOutput operation #11772

Merged

opoluektov-lohika force-pushed the fix-experimental-detectron-do-ref-impl branch from 39879c6 to 9d450f4 Compare May 31, 2022 15:39

tomdol approved these changes Jun 1, 2022

View reviewed changes

t-jankowski approved these changes Jun 1, 2022

View reviewed changes

docs/template_plugin/backend/evaluates_map.cpp Outdated Show resolved Hide resolved

opoluektov-lohika force-pushed the fix-experimental-detectron-do-ref-impl branch from 9d450f4 to bec1dfb Compare June 1, 2022 15:17

Oleksandr Poluektov added 3 commits June 6, 2022 16:47

ExperimentalDetectronDetectionOutput: reformat reference implementati…

78787f6

…on test for readability

ExperimentalDetectronDetectionOutput: fix reference implementation: c…

21131f0

…lasses Due to bug in indexing classes, 0-th class was not considered.

ExperimentalDetectronDetectionOutput: fix reference implementation: c…

7de13b8

…lip to image The resulting boxes must be clipped to the image according the op spec. Test case modified to cover the change in the implementation.

Oleksandr Poluektov added 6 commits June 6, 2022 16:47

ExperimentalDetectronDetectionOutput: enhance test case for reference…

d4c8f58

… implementation To check the output scores really belong to the corresponding classes.

ExperimentalDetectronDetectionOutput: fix reference implementation: R…

9e38c44

…OIs count It is not the same as max_detections_from_image as was incorrectly assumed in the code. Test case modified to cover the change in the implementation.

[CPU] Fix ExperimentalDetectronDetectionOutput operation

753857c

... to match reference implementation

ExperimentalDetectronDetectionOutput: change generated test inputs

8e730d3

So that the previous fixes in the operation implementations are actually tested.

Fix onnx test data for ExperimentalDetectronDetectoionOutput

1d9b38c

... accordingly to the changes in the reference implementation.

ExperimentalDetectronDetectionOutput: reformat back to make clang for…

e2bdb5b

…mat checcker happy

opoluektov-lohika force-pushed the fix-experimental-detectron-do-ref-impl branch from bec1dfb to e2bdb5b Compare June 6, 2022 13:47

tomdol enabled auto-merge (squash) June 8, 2022 07:51

tomdol merged commit d872338 into openvinotoolkit:master Jun 10, 2022

usstq reviewed Aug 19, 2022

View reviewed changes

jane-intel pushed a commit to jane-intel/openvino that referenced this pull request Aug 22, 2022

Revert "Fix experimental detectron do ref impl (openvinotoolkit#10621)"

734f615

This reverts commit d872338.

jane-intel pushed a commit to jane-intel/openvino that referenced this pull request Aug 26, 2022

Revert "Fix experimental detectron do ref impl (openvinotoolkit#10621)"

b05d7a1

This reverts commit d872338.

akladiev added a commit that referenced this pull request Aug 29, 2022

Revert "Revert "Fix experimental detectron do ref impl (#10621)" (#12683

65a0659

)" This reverts commit 4daee82.

jane-intel mentioned this pull request Sep 12, 2022

Revert "Fix experimental detectron do ref impl (#10621)" (#12683) #13009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix experimental detectron do ref impl #10621

Fix experimental detectron do ref impl #10621

opoluektov-lohika commented Feb 23, 2022 •

edited by tgubanova-lohika

Loading

tomdol left a comment

opoluektov-lohika commented May 18, 2022

tomdol commented May 19, 2022

opoluektov-lohika commented May 19, 2022

tomdol left a comment

usstq Aug 19, 2022 •

edited

Loading

opoluektov-lohika Aug 19, 2022

opoluektov-lohika Aug 19, 2022

usstq Aug 21, 2022

usstq Aug 21, 2022

jane-intel commented Aug 22, 2022

opoluektov-lohika commented Aug 22, 2022

Fix experimental detectron do ref impl #10621

Fix experimental detectron do ref impl #10621

Conversation

opoluektov-lohika commented Feb 23, 2022 • edited by tgubanova-lohika Loading

tomdol left a comment

Choose a reason for hiding this comment

opoluektov-lohika commented May 18, 2022

tomdol commented May 19, 2022

opoluektov-lohika commented May 19, 2022

tomdol left a comment

Choose a reason for hiding this comment

usstq Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

opoluektov-lohika Aug 19, 2022

Choose a reason for hiding this comment

opoluektov-lohika Aug 19, 2022

Choose a reason for hiding this comment

usstq Aug 21, 2022

Choose a reason for hiding this comment

usstq Aug 21, 2022

Choose a reason for hiding this comment

jane-intel commented Aug 22, 2022

opoluektov-lohika commented Aug 22, 2022

opoluektov-lohika commented Feb 23, 2022 •

edited by tgubanova-lohika

Loading

usstq Aug 19, 2022 •

edited

Loading