[G-API]: kmeans() Standard Kernel Implementation#18857
Merged
alalek merged 13 commits intoopencv:masterfrom Nov 30, 2020
Merged
[G-API]: kmeans() Standard Kernel Implementation#18857alalek merged 13 commits intoopencv:masterfrom
alalek merged 13 commits intoopencv:masterfrom
Conversation
- 4 overloads:
- standard GMat - for any dimensionality
- GMat without bestLabels initialization
- GArray<Point2f> - for 2D
- GArray<Point3f> - for 3D
- Accuracy tests:
- for every input - 2 tests
1) without initializing. In this case, no comparison with cv::kmeans is done as kmeans uses random auto-initialization
2) with initialization
- in both cases, only 1 attempt is done as after first attempt kmeans initializes bestLabels randomly
537576e to
3eb4c7f
Compare
Contributor
Author
|
@AsyaPronina please review |
alalek
reviewed
Nov 19, 2020
| // Checks if the passed mat is a set of n-dimentional points of the given depth | ||
| // Returns (quantity, elemSize) | ||
| std::tuple<int, int> checkVector(const int chan, const cv::Size &size, const int depth, | ||
| const int ddepth) |
Member
There was a problem hiding this comment.
I already saw similar functionality recently.
@dmatveev There is a lot of code going into the public headers:
- this may break bindings generators because they parse public headers
- this slowdowns compilation of OpenCV itself and user apps
There is strong recommendation to keep minimal amount of code in public headers.
Contributor
There was a problem hiding this comment.
In imgproc.hpp. @OrestChura, can you merge checkVector and isPointsVector?
Contributor
Author
There was a problem hiding this comment.
Thank you, Alexander, Maxim!
I made attempt to merge them and share the generalized function between gapi/core.hpp & gapi/imgproc.hpp. Put it in gmat.hpp/gmat.cpp
| // Checks if the passed mat is a set of n-dimentional points of the given depth | ||
| // Returns (quantity, elemSize) | ||
| std::tuple<int, int> checkVector(const int chan, const cv::Size &size, const int depth, | ||
| const int ddepth) |
Contributor
There was a problem hiding this comment.
In imgproc.hpp. @OrestChura, can you merge checkVector and isPointsVector?
- bestLabels is returned to its original place among parameters - checkVector and isPointsVector functions are merged into one, shared between core.hpp & imgproc.hpp by placing it into gmat.hpp (and implementation - to gmat.cpp) - typos corrected
AsyaPronina
reviewed
Nov 21, 2020
- unified names in tests - const added - typos
- fixed the doc note - ddepth -> expectedDepth, `< 0 ` -> `== -1`
- supported: multiple channels, reversed width - added test cases for those - added notes in docs - refactored checkVector to return dimentionality along with quantity
- makes chackVector smaller and (maybe) clearer
AsyaPronina
reviewed
Nov 25, 2020
mpashchenkov
approved these changes
Nov 25, 2020
Contributor
mpashchenkov
left a comment
There was a problem hiding this comment.
I hope it works better than correct.
fed4962 to
3d396c8
Compare
- cv::checkVector -> cv::gapi::detail
3d396c8 to
e1dd4d7
Compare
- Changed checkVector: returns bool, quantity & dimensionality as references
AsyaPronina
reviewed
Nov 26, 2020
AsyaPronina
reviewed
Nov 26, 2020
AsyaPronina
reviewed
Nov 26, 2020
AsyaPronina
reviewed
Nov 26, 2020
AsyaPronina
reviewed
Nov 26, 2020
- Polishing checkVector - FIXME added
00ccc5c to
38952b2
Compare
- checkVector: added overload, separate two different functionalities - depth assert - out of the function
38952b2 to
3a91f5e
Compare
AsyaPronina
approved these changes
Nov 27, 2020
Contributor
|
Thanks a lot for your efforts, Orest!! |
- quantity -> amount, dimensionality -> dim - Fix typos
741718a to
815c120
Compare
alalek
reviewed
Nov 29, 2020
| outMeta(const GMatDesc& in, int K, const GMatDesc& bestLabels, const TermCriteria&, int, | ||
| KmeansFlags flags) { | ||
| GAPI_Assert(in.depth == CV_32F); | ||
| std::vector<int> amount_n_dim = detail::checkVector(in); |
Member
There was a problem hiding this comment.
@dmatveev We need new approach to get rid implementation code from public headers.
Keep interface definition only.
This also should get rid several helper functions.
Contributor
Author
There was a problem hiding this comment.
I see your concerns about implementations in .hpp, will discuss it within the team. I think this has to be solved for all the kernels in our modules. Although, I think this step is not for this PR, now I propose to leave it as is for consistency and then move all the implementations at once.
But what do you mean by helper functions?
Comment on lines
+529
to
+532
| // kmeans sets these labels' sizes when no bestLabels given: | ||
| GMatDesc out_labels(CV_32S, 1, Size{1, amount}), | ||
| // kmeans always sets these centers' sizes: | ||
| centers (CV_32F, 1, Size{dim, K}); |
Member
There was a problem hiding this comment.
Please use 2 variable's definitions instead of one (for all non-trivial variables).
This is necessary for:
- 2 dedicated lines rovided by compiler
- 2 lines in debugger
- 2 lines in valgrind reports in case of memory issues.
(+ fix indentation of comments)
// kmeans sets these labels' sizes when no bestLabels given:
GMatDesc out_labels(CV_32S, 1, Size{1, amount});
// kmeans always sets these centers' sizes:
GMatDesc centers (CV_32F, 1, Size{dim, K});
Contributor
Author
There was a problem hiding this comment.
OK, thanks. Fixed
| or N columns if there is a single channel. Mat should have @ref CV_32F depth. | ||
|
|
||
| @note Although, if GMat with height != 1, width != 1, channels != 1 given as data, n-dimensional | ||
| samples are considered given in amount of A, where A = height, n = width * channels. |
Member
There was a problem hiding this comment.
Move @note outside of @params section.
Check the documentation page.
Contributor
Author
There was a problem hiding this comment.
OK, I see. Changed the notes
Comment on lines
+1862
to
+1867
| @return Compactness measure that is computed as | ||
| \f[\sum _i \| \texttt{samples} _i - \texttt{centers} _{ \texttt{labels} _i} \| ^2\f] | ||
| after every attempt. The best (minimum) value is chosen and the corresponding labels and the | ||
| compactness value are returned by the function. | ||
| @return Integer array that stores the cluster indices for every sample. | ||
| @return Array of the cluster centers. |
Member
There was a problem hiding this comment.
@return must be used once.
Move all details/notes before @params.
modules/gapi/src/api/gmat.cpp
Outdated
| } | ||
|
|
||
| namespace { | ||
| std::vector<int> checkVectorImpl(const int width, const int height, const int chan, const int n) |
Member
There was a problem hiding this comment.
namespace {
may be just "static"?
(to avoid unnecessary obfuscated names in debugger)
Contributor
Author
There was a problem hiding this comment.
Agree. Thanks! Changed
- fix docs - use 2 variable's definitions instead of one (for all non-trivial variables)
alalek
reviewed
Dec 7, 2020
| { | ||
| EXPECT_TRUE(compact_gapi == compact_ocv); | ||
| EXPECT_TRUE(compareVectorsAbsExact(labels_gapi, labels_ocv)); | ||
| EXPECT_TRUE(compareVectorsAbsExact(centers_gapi, centers_ocv)); |
Member
There was a problem hiding this comment.
there are several reports from coverity tool:
>>> CID 1470409: (SWAPPED_ARGUMENTS)
>>> The positions of arguments in the call to "compareVectorsAbsExact" do not match the ordering of the parameters:
* "centers_gapi" is passed to "outOCV"
* "centers_ocv" is passed to "outGAPI"
1567 EXPECT_TRUE(compareVectorsAbsExact(centers_gapi, centers_ocv));
please take a look
4 tasks
Merged
a-sajjad72
pushed a commit
to a-sajjad72/opencv
that referenced
this pull request
Mar 30, 2023
[G-API]: kmeans() Standard Kernel Implementation
* cv::gapi::kmeans kernel implementation
- 4 overloads:
- standard GMat - for any dimensionality
- GMat without bestLabels initialization
- GArray<Point2f> - for 2D
- GArray<Point3f> - for 3D
- Accuracy tests:
- for every input - 2 tests
1) without initializing. In this case, no comparison with cv::kmeans is done as kmeans uses random auto-initialization
2) with initialization
- in both cases, only 1 attempt is done as after first attempt kmeans initializes bestLabels randomly
* Addressing comments
- bestLabels is returned to its original place among parameters
- checkVector and isPointsVector functions are merged into one, shared between core.hpp & imgproc.hpp by placing it into gmat.hpp (and implementation - to gmat.cpp)
- typos corrected
* addressing comments
- unified names in tests
- const added
- typos
* Addressing comments
- fixed the doc note
- ddepth -> expectedDepth, `< 0 ` -> `== -1`
* Fix unsupported cases of input Mat
- supported: multiple channels, reversed width
- added test cases for those
- added notes in docs
- refactored checkVector to return dimentionality along with quantity
* Addressing comments
- makes chackVector smaller and (maybe) clearer
* Addressing comments
* Addressing comments
- cv::checkVector -> cv::gapi::detail
* Addressing comments
- Changed checkVector: returns bool, quantity & dimensionality as references
* Addressing comments
- Polishing checkVector
- FIXME added
* Addressing discussion
- checkVector: added overload, separate two different functionalities
- depth assert - out of the function
* Addressing comments
- quantity -> amount, dimensionality -> dim
- Fix typos
* Addressing comments
- fix docs
- use 2 variable's definitions instead of one (for all non-trivial variables)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes:
kmeans()standard kernelGMat- for any dimensionalityGMatwithout bestLabels initialization for convenienceGArray<Point2f>- for 2DGArray<Point3f>- for 3DPull 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.