| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
G-API: Added reshape() functionality to CPU backend #21669
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to expand tests for stateful kernels with reshape test?
rgarnov
force-pushed
the
rg/cpu_backend_reshape
branch
from
ee2cfd3 to
3d46ee2
Compare
@AsyaPronina, added an appropriate test, thanks! |
| @@ -2,7 +2,7 @@ | |||
| // It is subject to the license terms in the LICENSE file found in the top-level directory | |||
| // of this distribution and at https://opencv.org/license.html. | |||
| // | |||
| // Copyright (C) 2018-2020 Intel Corporation | |||
| // Copyright (C) 2018-2021 Intel Corporation | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| } | ||
| } | ||
|
|
||
| void cv::gimpl::GCPUExecutable::reshape(ade::Graph&, const GCompileArgs&) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it's needed to handle GCompileArgs as well, because stateful kernels use them:
https://github.com/opencv/opencv/blob/4.x/modules/gapi/src/backends/cpu/gcpubackend.cpp#L175
I'd propose to add test on that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
rgarnov
force-pushed
the
rg/cpu_backend_reshape
branch
from
3d46ee2 to
06084a8
Compare
rgarnov
force-pushed
the
rg/cpu_backend_reshape
branch
from
06084a8 to
3203393
Compare
rgarnov
force-pushed
the
rg/cpu_backend_reshape
branch
from
3203393 to
ecb3040
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
đź‘Ť overall LGTM
| break; | ||
| } | ||
| default: util::throw_error(std::logic_error("Unsupported NodeType type")); | ||
| } | ||
| } | ||
|
|
||
| makeReshape(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this method call should be commented in the same way as the below one -- to clearly outline its intention and why it is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your efforts!!
Thanks a lot, perfect tests!! |
|
@alalek could you proceed with the merge please? |
Summary
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
Build configuration