Skip to content

Support for Pool1d layer for OpenCV and OpenCL targets#18862

Merged
alalek merged 8 commits intoopencv:3.4from
sl-sergei:support_pool1d
Nov 24, 2020
Merged

Support for Pool1d layer for OpenCV and OpenCL targets#18862
alalek merged 8 commits intoopencv:3.4from
sl-sergei:support_pool1d

Conversation

@sl-sergei
Copy link
Copy Markdown
Contributor

@sl-sergei sl-sergei commented Nov 19, 2020 •
edited
Loading

Resolves: #18643

Merge with extra: opencv/opencv_extra#820

opencv_extra=test_pool1d

force_builders=Custom,Custom Win,Custom Mac
build_image:Custom=ubuntu-openvino-2021.1.0:20.04
build_image:Custom Win=openvino-2021.1.0
build_image:Custom Mac=openvino-2021.1.0

test_modules:Custom=dnn,python2,python3,java
test_modules:Custom Win=dnn,python2,python3,java
test_modules:Custom Mac=dnn,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

@sl-sergei sl-sergei added this to the 3.4.13 milestone Nov 19, 2020
dkurt
dkurt reviewed Nov 19, 2020
View reviewed changes
@dkurt dkurt self-assigned this Nov 23, 2020
@sl-sergei
Copy link
Copy Markdown
Contributor Author

sl-sergei commented Nov 23, 2020 •
edited
Loading

@dkurt @alalek I changed the code for Halide backend a little bit, it does seem correct, but do our tests cover this code?

dkurt
dkurt reviewed Nov 23, 2020
View reviewed changes
@sl-sergei sl-sergei changed the title WIP: Support Pool1d Support for Pool1d layer for OpenCV and OpenCL targets Nov 23, 2020
@sl-sergei
Copy link
Copy Markdown
Contributor Author

sl-sergei commented Nov 24, 2020

@dkurt Can you please review the recent changes?

alalek
alalek reviewed Nov 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for contribution!

modules/dnn/src/layers/pooling_layer.cpp Outdated
if (kernel_size.size() == 1)
return (backendId == DNN_BACKEND_OPENCV && preferableTarget == DNN_TARGET_CPU) ||
(backendId == DNN_BACKEND_OPENCV && preferableTarget == DNN_TARGET_OPENCL) ||
(backendId == DNN_BACKEND_OPENCV && preferableTarget == DNN_TARGET_OPENCL_FP16);
Copy link
Copy Markdown
Member

@alalek alalek Nov 24, 2020

Choose a reason for hiding this comment

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

supportBackend(int backendId)

is not supposed to check preferableTarget value at all.

It is a design bug.
Caller doesn't expect failures for DNN_BACKEND_OPENCV.

/cc @dkurt

return backendId == DNN_BACKEND_OPENCV; should be enough here (replace these 3 lines).

Copy link
Copy Markdown
Member

@dkurt dkurt Nov 24, 2020

Choose a reason for hiding this comment

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

Actually, there is already else if (backendId == DNN_BACKEND_OPENCV || backendId == DNN_BACKEND_HALIDE) so this condition is always true

Copy link
Copy Markdown
Member

@alalek alalek Nov 24, 2020

Choose a reason for hiding this comment

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

It is false if backendId == DNN_BACKEND_HALIDE.

@dkurt
Copy link
Copy Markdown
Member

dkurt commented Nov 24, 2020

Please take a look at failed test: https://pullrequest.opencv.org/buildbot/builders/precommit_custom_mac/builds/1440.

dkurt
dkurt reviewed Nov 24, 2020
View reviewed changes
modules/dnn/src/layers/pooling_layer.cpp Outdated
if (kernel_size.size() == 1)
return (preferableTarget == DNN_TARGET_CPU ||
preferableTarget == DNN_TARGET_OPENCL ||
preferableTarget == DNN_TARGET_OPENCL_FP16);
Copy link
Copy Markdown
Member

@dkurt dkurt Nov 24, 2020

Choose a reason for hiding this comment

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

Do we have other targets?

Copy link
Copy Markdown
Contributor Author

@sl-sergei sl-sergei Nov 24, 2020

Choose a reason for hiding this comment

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

Do we have other targets?

I just try to revert my changes to see why the tests broke. Everything was fine until I changed logic for support checks. See, for instance, https://pullrequest.opencv.org/buildbot/builders/precommit_custom_mac/builds/1438

@sl-sergei
Copy link
Copy Markdown
Contributor Author

sl-sergei commented Nov 24, 2020

@alalek Do you know what is wrong with Cmake warnings on Mac?
https://pullrequest.opencv.org/buildbot/builders/precommit_custom_mac/builds/1441/steps/cmake/logs/warnings%20%281%29

@alalek
Copy link
Copy Markdown
Member

alalek commented Nov 24, 2020

Cmake warnings on Mac

ignore, not related to this PR

dkurt
dkurt approved these changes Nov 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@dkurt dkurt left a comment

Choose a reason for hiding this comment

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

👍 Thank you!

@alalek alalek merged commit f4f462c into opencv:3.4 Nov 24, 2020
This was referenced Nov 27, 2020
Merged
Merged
@sl-sergei sl-sergei mentioned this pull request Nov 30, 2020
Merged
@sl-sergei sl-sergei mentioned this pull request Dec 8, 2020
Merged
@sl-sergei sl-sergei mentioned this pull request Dec 8, 2020
Merged
@alalek alalek mentioned this pull request Dec 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek left review comments

@dkurt dkurt dkurt approved these changes

Assignees

@dkurt dkurt

Labels

category: dnn feature

Projects

None yet

Milestone

3.4.13

Development

Successfully merging this pull request may close these issues.

3 participants

@sl-sergei @dkurt @alalek