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

Fix deformable_convolution #6393

Merged

Conversation

DingZhangIntel
Copy link
Contributor

I think there is a error about the reference implementation of deformable_convolution.
The two parameters group and deformable_group is independent, and there are details about the parameter deformable_group in MXNet
Suppose that the shape of input data is (1,6,5,5), the shape of filter is (2,3,3,3), the shape of the offset is (1,54,3,3), group is 2, deformable_group is 3. And an error will occur on line 256 when group_idx equals 1.

@DingZhangIntel DingZhangIntel requested a review from a team June 28, 2021 03:21
@openvino-pushbot openvino-pushbot added ExternalIntelPR External contributor from Intel category: Core OpenVINO Core (aka ngraph) labels Jun 28, 2021
@ilyachur
Copy link
Contributor

@DingZhangIntel Could you please add new tests?

@ilyachur ilyachur added the pr: needs tests PR needs tests updating label Jun 29, 2021
@ilyachur ilyachur added this to the 2022.1 milestone Jun 29, 2021
@DingZhangIntel
Copy link
Contributor Author

@DingZhangIntel Could you please add new tests

@DingZhangIntel DingZhangIntel requested a review from a team June 29, 2021 08:43
@DingZhangIntel
Copy link
Contributor Author

DingZhangIntel commented Jun 29, 2021

@DingZhangIntel Could you please add new tests

I have added a single test case in the latest commit, but I don't know whether this is what you want. If it is, I think it is a simple example to explain why I made these changes. If not, please contact me at any your time, I will give more details about this PR.

@ilyachur
Copy link
Contributor

@DingZhangIntel Could you please add new tests

I have added a single test case in the latest commit, but I don't know whether this is what you want. If it is, I think it is a simple example to explain why I made these changes. If not, please contact me at any your time, I will give more details about this PR.

@DingZhangIntel Looks like new tests are failing.

@DingZhangIntel DingZhangIntel force-pushed the deformable_convolution branch from cead7cc to 7fc16e5 Compare July 2, 2021 08:37
@DingZhangIntel
Copy link
Contributor Author

DingZhangIntel commented Jul 2, 2021

@DingZhangIntel Could you please add new tests

I have added a single test case in the latest commit, but I don't know whether this is what you want. If it is, I think it is a simple example to explain why I made these changes. If not, please contact me at any your time, I will give more details about this PR.

@DingZhangIntel Looks like new tests are failing.

I just made a little changes in the origin test file, but it still fails on CI. I think it's a little weird. I will very appreciate it if you can check these changes at your convenience. I will keep looking into this issue, thanks.

@ilyachur
Copy link
Contributor

ilyachur commented Jul 5, 2021

@DingZhangIntel Could you please add new tests

I have added a single test case in the latest commit, but I don't know whether this is what you want. If it is, I think it is a simple example to explain why I made these changes. If not, please contact me at any your time, I will give more details about this PR.

@DingZhangIntel Looks like new tests are failing.

I just made a little changes in the origin test file, but it still fails on CI. I think it's a little weird. I will very appreciate it if you can check these changes at your convenience. I will keep looking into this issue, thanks.

As I see the CPU plugin cannot create a network for your tests:

[ RUN      ] smoke_DeformableConvolution2D_DeformableGroups_ExplicitPadding/DeformableConvolutionLayerTest.CompareWithRefs/IS=(1.6.30.30)_DV(1.54.28.28)_K(2.3.3.3)_S(1.1)_PB(0.0)_PE(0.0)_D=(1.1)_G=2_DG=3_O=5_AP=explicit_netPRC=I32_inPRC=UNSPECIFIED_outPRC=UNSPECIFIED_inL=ANY_outL=ANY_trgDev=CPU

MEM_USAGE=79613952KB
C:\a\1\openvino\inference-engine\tests\functional\shared_test_classes\src\base\layer_test_utils.cpp(57): error: could not construct a memory descriptor using a format tag
[  FAILED  ] smoke_DeformableConvolution2D_DeformableGroups_ExplicitPadding/DeformableConvolutionLayerTest.CompareWithRefs/IS=(1.6.30.30)_DV(1.54.28.28)_K(2.3.3.3)_S(1.1)_PB(0.0)_PE(0.0)_D=(1.1)_G=2_DG=3_O=5_AP=explicit_netPRC=I32_inPRC=UNSPECIFIED_outPRC=UNSPECIFIED_inL=ANY_outL=ANY_trgDev=CPU (13 ms)

@DingZhangIntel
Copy link
Contributor Author

@DingZhangIntel Could you please add new tests

I have added a single test case in the latest commit, but I don't know whether this is what you want. If it is, I think it is a simple example to explain why I made these changes. If not, please contact me at any your time, I will give more details about this PR.

@DingZhangIntel Looks like new tests are failing.

I just made a little changes in the origin test file, but it still fails on CI. I think it's a little weird. I will very appreciate it if you can check these changes at your convenience. I will keep looking into this issue, thanks.

As I see the CPU plugin cannot create a network for your tests:

[ RUN      ] smoke_DeformableConvolution2D_DeformableGroups_ExplicitPadding/DeformableConvolutionLayerTest.CompareWithRefs/IS=(1.6.30.30)_DV(1.54.28.28)_K(2.3.3.3)_S(1.1)_PB(0.0)_PE(0.0)_D=(1.1)_G=2_DG=3_O=5_AP=explicit_netPRC=I32_inPRC=UNSPECIFIED_outPRC=UNSPECIFIED_inL=ANY_outL=ANY_trgDev=CPU

MEM_USAGE=79613952KB
C:\a\1\openvino\inference-engine\tests\functional\shared_test_classes\src\base\layer_test_utils.cpp(57): error: could not construct a memory descriptor using a format tag
[  FAILED  ] smoke_DeformableConvolution2D_DeformableGroups_ExplicitPadding/DeformableConvolutionLayerTest.CompareWithRefs/IS=(1.6.30.30)_DV(1.54.28.28)_K(2.3.3.3)_S(1.1)_PB(0.0)_PE(0.0)_D=(1.1)_G=2_DG=3_O=5_AP=explicit_netPRC=I32_inPRC=UNSPECIFIED_outPRC=UNSPECIFIED_inL=ANY_outL=ANY_trgDev=CPU (13 ms)

I just tested it in my local openvino repo and found that if i set the parameter group equals 2, the test will fail. And if I set the parameter group equals 1, the test will work. please check.

@DingZhangIntel
Copy link
Contributor Author

DingZhangIntel commented Jul 5, 2021

@DingZhangIntel Could you please add new tests

I have added a single test case in the latest commit, but I don't know whether this is what you want. If it is, I think it is a simple example to explain why I made these changes. If not, please contact me at any your time, I will give more details about this PR.

@DingZhangIntel Looks like new tests are failing.

I just made a little changes in the origin test file, but it still fails on CI. I think it's a little weird. I will very appreciate it if you can check these changes at your convenience. I will keep looking into this issue, thanks.

As I see the CPU plugin cannot create a network for your tests:

[ RUN      ] smoke_DeformableConvolution2D_DeformableGroups_ExplicitPadding/DeformableConvolutionLayerTest.CompareWithRefs/IS=(1.6.30.30)_DV(1.54.28.28)_K(2.3.3.3)_S(1.1)_PB(0.0)_PE(0.0)_D=(1.1)_G=2_DG=3_O=5_AP=explicit_netPRC=I32_inPRC=UNSPECIFIED_outPRC=UNSPECIFIED_inL=ANY_outL=ANY_trgDev=CPU

MEM_USAGE=79613952KB
C:\a\1\openvino\inference-engine\tests\functional\shared_test_classes\src\base\layer_test_utils.cpp(57): error: could not construct a memory descriptor using a format tag
[  FAILED  ] smoke_DeformableConvolution2D_DeformableGroups_ExplicitPadding/DeformableConvolutionLayerTest.CompareWithRefs/IS=(1.6.30.30)_DV(1.54.28.28)_K(2.3.3.3)_S(1.1)_PB(0.0)_PE(0.0)_D=(1.1)_G=2_DG=3_O=5_AP=explicit_netPRC=I32_inPRC=UNSPECIFIED_outPRC=UNSPECIFIED_inL=ANY_outL=ANY_trgDev=CPU (13 ms)

I just tested it in my local openvino repo and found that if i set the parameter group equals 2, the test will fail. And if I set the parameter group equals 1, the test will work. please check.

I tested it again and found that if I set the parameter group greater than 1, the test will fail. Please check.

@DingZhangIntel
Copy link
Contributor Author

DingZhangIntel commented Jul 5, 2021

@DingZhangIntel Could you please add new tests

I have added a single test case in the latest commit, but I don't know whether this is what you want. If it is, I think it is a simple example to explain why I made these changes. If not, please contact me at any your time, I will give more details about this PR.

@DingZhangIntel Looks like new tests are failing.

I just made a little changes in the origin test file, but it still fails on CI. I think it's a little weird. I will very appreciate it if you can check these changes at your convenience. I will keep looking into this issue, thanks.

As I see the CPU plugin cannot create a network for your tests:

[ RUN      ] smoke_DeformableConvolution2D_DeformableGroups_ExplicitPadding/DeformableConvolutionLayerTest.CompareWithRefs/IS=(1.6.30.30)_DV(1.54.28.28)_K(2.3.3.3)_S(1.1)_PB(0.0)_PE(0.0)_D=(1.1)_G=2_DG=3_O=5_AP=explicit_netPRC=I32_inPRC=UNSPECIFIED_outPRC=UNSPECIFIED_inL=ANY_outL=ANY_trgDev=CPU

MEM_USAGE=79613952KB
C:\a\1\openvino\inference-engine\tests\functional\shared_test_classes\src\base\layer_test_utils.cpp(57): error: could not construct a memory descriptor using a format tag
[  FAILED  ] smoke_DeformableConvolution2D_DeformableGroups_ExplicitPadding/DeformableConvolutionLayerTest.CompareWithRefs/IS=(1.6.30.30)_DV(1.54.28.28)_K(2.3.3.3)_S(1.1)_PB(0.0)_PE(0.0)_D=(1.1)_G=2_DG=3_O=5_AP=explicit_netPRC=I32_inPRC=UNSPECIFIED_outPRC=UNSPECIFIED_inL=ANY_outL=ANY_trgDev=CPU (13 ms)

I just tested it in my local openvino repo and found that if i set the parameter group equals 2, the test will fail. And if I set the parameter group equals 1, the test will work. please check.

I tested it again and found that if I set the parameter group greater than 1, the test will fail. Please check.

I changed the tests back and added a single test case to explain why I made these changes in ref implementation, and the single test case will produce the mismatch results comparing with MXNet. The error will occur on line 256 of the origin ref implementation, the offset of the variable group_offsets is too big. Please check.

@ilyachur
Copy link
Contributor

ilyachur commented Jul 6, 2021

@itikhono Please have a look.

@itikhono itikhono self-requested a review July 6, 2021 10:24
@itikhono
Copy link
Contributor

itikhono commented Jul 6, 2021

hello @DingZhangIntel,

As understood, you encounted 2 issues:

  • CPU plugin doesn't work with group = 2, deformable_group = 3 attributes of DeformableConvolution. I re-checked a backlog and found the similar ticket (cpu impl doesn't work for group > 1), I guess the bug will be fixed in this Release.
  • a bug in the reference implementation: thanks for your fix, it looks correct, for some reason the current reference works with group = 2, deformable_group = 2 attributes, so we didn't find the problem earlier, it reproducible for deformable_group >2

@ilyachur ilyachur enabled auto-merge (squash) July 6, 2021 11:56
@ilyachur ilyachur merged commit 32897ab into openvinotoolkit:master Jul 6, 2021
@YangleiZouIntel YangleiZouIntel deleted the deformable_convolution branch July 7, 2021 02:54
andrew-k-park pushed a commit to andrew-k-park/openvino that referenced this pull request Jul 14, 2021
* Fix deformable_convolution

* Fix unused-variable

* Change a test case

* Change tests back and add single test case
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
* Fix deformable_convolution

* Fix unused-variable

* Change a test case

* Change tests back and add single test case
andrei-cv pushed a commit to andrei-cv/openvino that referenced this pull request Aug 30, 2021
* Fix deformable_convolution

* Fix unused-variable

* Change a test case

* Change tests back and add single test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph) ExternalIntelPR External contributor from Intel pr: needs tests PR needs tests updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants