CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
G-API: Support vaargs for cv.compile_args #20196
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
G-API: Support vaargs for cv.compile_args #20196
Conversation
@dmatveev @mpashchenkov @sivanov-work Folks, have a look, please |
@alalek Compatibility with |
GAPI_EXPORTS_W GCompileArgs compile_args(gapi::GNetPackage pkg); | ||
GAPI_EXPORTS_W GCompileArgs compile_args(gapi::GKernelPackage kernels, gapi::GNetPackage nets); | ||
struct GAPI_EXPORTS_W_SIMPLE GCompileArg { | ||
GAPI_WRAP GCompileArg(gapi::GKernelPackage pkg); |
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.
To extend number of possible types from another module need to do:
// shadow_somemodule.hpp
struct GCompileArg {
GAPI_WRAP GCompileArg(module::SomeArg arg);
};
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.
This code adds new overload to GCompileArg
ctor
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.
But how did you solve variadic arguments passing?
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.
it's solved indirectly through python's compile_args
(which probably requires a better PR name since it's confusing - vaargs is a thing in C++ as well).
map()
calls a function for each element of a range. given that the function is a GCompileArg
ctor, we convert "raw" args into a range containing cv2.GCompileArg(arg)
elements (by the way, does ctor qualify as a function?)
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.
by the way, does ctor qualify as a function
Why not ? https://stackoverflow.com/questions/11943831/python-how-to-put-constructors-in-map-function
GAPI_EXPORTS_W GCompileArgs compile_args(gapi::GNetPackage pkg); | ||
GAPI_EXPORTS_W GCompileArgs compile_args(gapi::GKernelPackage kernels, gapi::GNetPackage nets); | ||
struct GAPI_EXPORTS_W_SIMPLE GCompileArg { | ||
GAPI_WRAP GCompileArg(gapi::GKernelPackage pkg); |
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.
it's solved indirectly through python's compile_args
(which probably requires a better PR name since it's confusing - vaargs is a thing in C++ as well).
map()
calls a function for each element of a range. given that the function is a GCompileArg
ctor, we convert "raw" args into a range containing cv2.GCompileArg(arg)
elements (by the way, does ctor qualify as a function?)
struct GAPI_EXPORTS_W_SIMPLE GCompileArg { | ||
GAPI_WRAP GCompileArg(gapi::GKernelPackage pkg); | ||
GAPI_WRAP GCompileArg(gapi::GNetPackage pkg); | ||
}; |
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.
while I like the idea, this API is not quite equivalent. what if, as a user, I have my own gapi::GMyOwnPackage
- how can I use compile_args
machinery to wrap this external-to-G-API package?
it seems remotely possible with the old API (just add a new overload of compile_args
) - albeit, I'm not sure if python has anything like function overload resolution/adl (wondering how this old API works) - if this is out of scope for this (e.g. G-API only supports packages that it provides itself via compile_args
and users have to do something else), then feel free to ignore this.
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.
Wow, glad to see u :)
As for gapi::GMyOwnPackage
what do u mean ? Do u want to use your own compile argument ?
From python user can't define new compile argument.
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.
Wow, glad to see u :)
Just occasionally looking at things here, nothing too serious 😉
As for gapi::GMyOwnPackage what do u mean ? Do u want to use your own compile argument ?
Yes, imagine such a use case (maybe someone wants their own package - it is the thing that provides the kernels/etc, right? can't remember anymore).
From python user can't define new compile argument.
Well, it should be doable from C++ and then C++ gets magically converted into python - I guess, ideally, G-API should be convenient to use from python as well, so if someone supplies their own package, it could be used from python API as well.
However, this is a purely artificial use case. I wrote this comment in the meaning of:
you change the API and this (might) limit the usability - e.g. the ability to use custom packages in python. if this was never a concern, then this change is perfectly fine.
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.
(glad to see you as well!)
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.
Yes, imagine such a use case (maybe someone wants their own package - it is the thing that provides the kernels/etc, right? can't remember anymore).
Again, what's the purpose of the gapi::GMyOwnPackage
? First of all it won't be recognized inside G-API, right ? Secondly if the user want to pass his own set of kernels it can be easily done by using cv.gapi.kernels(....)
which returns gapi::GKernelPackage
I mean that user defined compile arguments are useless because there is no support inside G-API to handle them.
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.
ah, I get it now. no objections then.
…port-vaargs-compile-args
self.assertEqual(centers.shape[1], sz[1]); | ||
self.assertEqual(centers.shape[0], K); | ||
self.assertTrue(centers.size != 0); |
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.
;
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
6f8d6e7
to
ca3787d
Compare
@alalek Can it be merged ? |
@@ -11,6 +11,11 @@ def parameterized(func): | |||
return parameterized | |||
|
|||
|
|||
@register('cv2') | |||
def compile_args(*args): |
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.
cv2 => cv2.gapi ?
(as there is no "G" prefix)
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.
but in c++
it's a part of cv
namespace, not cv::gapi
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.
It will be a bit weird to keep it in cv.gapi
package in that case. Are u Ok to leave it as is ?
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.
@dmatveev Please confirm this usage.
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.
@alalek yes somehow we have cv::compile_args
but not cv::gapi::compile_args
in our C++ API.
It is definitely worth aligning in 5.0
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.
Then it makes sense to keep it in cv.gapi
for python.
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
…-compile-args G-API: Support vaargs for cv.compile_args * Support cv.compile_args to work with variadic number of inputs * Disable python2.x G-API * Move compile_args to gapi pkg
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