Skip to content

Preparations for enabling 1D support for Conv and Pool operations on CUDA#19057

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
sl-sergei:changes_for_cuda_1d
Jan 30, 2021
Merged

Preparations for enabling 1D support for Conv and Pool operations on CUDA#19057
opencv-pushbot merged 1 commit intoopencv:3.4from
sl-sergei:changes_for_cuda_1d

Conversation

@sl-sergei
Copy link
Copy Markdown
Contributor

@sl-sergei sl-sergei commented Dec 8, 2020 •
edited by alalek
Loading

relates: #18862 #18783

Necessary changes for enabling Conv1D and Pool1D for CUDA in master branch.

force_builders=Custom,Custom Win,Custom Mac
buildworker:Custom=linux-4
build_image:Custom=ubuntu-cuda:16.04

Xbuild_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

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

Xbuildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
Xtest_opencl:Custom=OFF
Xtest_bigdata:Custom=1
Xtest_filter:Custom=*

@sl-sergei sl-sergei requested a review from alalek December 8, 2020 18:50
@sl-sergei sl-sergei mentioned this pull request Dec 8, 2020
Merged
@sl-sergei sl-sergei added this to the 3.4.13 milestone Dec 8, 2020
@sl-sergei sl-sergei requested a review from dkurt December 8, 2020 19:55
dkurt
dkurt reviewed Dec 9, 2020
View reviewed changes
modules/dnn/src/caffe/caffe_importer.cpp Outdated
String name = layer.name();
String type = layer.type();
LayerParams layerParams;
layerParams.set("framework", "caffe");
Copy link
Copy Markdown
Member

@dkurt dkurt Dec 9, 2020

Choose a reason for hiding this comment

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

Can we avoid this flag?

Copy link
Copy Markdown
Member

@alalek alalek Dec 9, 2020

Choose a reason for hiding this comment

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

Agree.
Lets don't push information about frameworks on "execution" level. Let importers only to handle framework specifics.

If it is necessary, please add another flag which describes behavior instead of framework.

P.S. Especially we don't want to handle framework versions during the execution.

dkurt
dkurt reviewed Dec 9, 2020
View reviewed changes
modules/dnn/src/layers/layers_common.cpp Outdated
}
if (parameter.size() == 1)
parameter.resize(2, parameter[0]);

Copy link
Copy Markdown
Member

@dkurt dkurt Dec 9, 2020

Choose a reason for hiding this comment

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

extra new line

modules/dnn/test/test_layers.cpp Outdated
}

layer->finalize(inp, outp);
layer->finalize(inp, outp);
Copy link
Copy Markdown
Member

@dkurt dkurt Dec 9, 2020

Choose a reason for hiding this comment

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

please fix indentation

modules/dnn/test/test_layers.cpp Outdated
lp.set("group", group);
lp.set("stride", 1);
std::vector<int> stride(2, 1);
lp.set("stride", DictValue::arrayInt<int*>(stride.data(), stride.size()));
Copy link
Copy Markdown
Member

@dkurt dkurt Dec 9, 2020

Choose a reason for hiding this comment

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

PR going to support Conv1D for CUDA but we already integrated Conv1D and it works with single value paddings, strides and kernel_size. So why for this backend we need to change common part?

Copy link
Copy Markdown
Contributor Author

@sl-sergei sl-sergei Dec 10, 2020

Choose a reason for hiding this comment

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

PR going to support Conv1D for CUDA but we already integrated Conv1D and it works with single value paddings, strides and kernel_size. So why for this backend we need to change common part?

This changes are needed for CUDA, but I think we should have the similar CUDA-independent logic for master and 3.4 branches

Copy link
Copy Markdown
Member

@alalek alalek Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How changes in tests are required to support specific backend?
Necessary logic should be handled by backend implementation itself.

Please put this on hold and complete #19058 first to see scope of the problem.

Copy link
Copy Markdown
Contributor Author

@sl-sergei sl-sergei Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

changes in tests

I removed changes in tests. I just want to preserve the same code and logic for backend-independent parts in master and 3.4

@sl-sergei sl-sergei force-pushed the changes_for_cuda_1d branch 2 times, most recently from 9c258ff to 5e562fd Compare December 10, 2020 15:20
@asmorkalov asmorkalov requested a review from dkurt January 18, 2021 11:35
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jan 18, 2021

@alalek @dkurt Please take a look.

alalek
alalek reviewed Jan 22, 2021
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.

Need to re-check applied changes from https://github.com/opencv/opencv/pull/19058/files

modules/dnn/src/layers/convolution_layer.cpp Outdated
#define REMOVE_EXTRA_ELEM(vector)\
if (vector.size() > 1)\
vector.pop_back();
REMOVE_EXTRA_ELEM(kernel_size);
Copy link
Copy Markdown
Member

@alalek alalek Jan 22, 2021

Choose a reason for hiding this comment

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

There is no such code on "master" branch.

Copy link
Copy Markdown
Member

@alalek alalek Jan 29, 2021

Choose a reason for hiding this comment

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

@sl-sergei Any feedback?

Copy link
Copy Markdown
Contributor Author

@sl-sergei sl-sergei Jan 29, 2021

Choose a reason for hiding this comment

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

@sl-sergei Any feedback?

@alalek I updated it, now everything should be ok

@sl-sergei sl-sergei force-pushed the changes_for_cuda_1d branch from 297f776 to e2949c7 Compare January 29, 2021 20:49
@sl-sergei sl-sergei requested a review from alalek January 29, 2021 20:50
alalek
alalek approved these changes Jan 30, 2021
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 👍

@opencv-pushbot opencv-pushbot merged commit b9dfffc into opencv:3.4 Jan 30, 2021
@alalek alalek mentioned this pull request Jan 31, 2021
Merged
@alalek alalek mentioned this pull request Apr 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek approved these changes

@dkurt dkurt Awaiting requested review from dkurt

Assignees

@alalek alalek

Labels

category: dnn port/backport done Label for maintainers. Authors of PR can ignore this

Projects

None yet

Milestone

3.4.14

Development

Successfully merging this pull request may close these issues.

5 participants

@sl-sergei @asmorkalov @alalek @dkurt @opencv-pushbot