[G-API]: Performance tests for KalmanFilter by OrestChura · Pull Request #19712 · opencv/opencv

OrestChura · 2021-03-11T15:12:47Z

Performance tests for KalmanFilter stateful operation added
Accuracy test misprint fixed and a little refactoring applied
A bug with changing KalmanParams due to non-copy assignment in KalmanFilter::setup() fixed

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

force_builders=Custom
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

xbuild_image:Custom=ubuntu-openvino-2021.1.0:20.04
xbuild_image:Custom Win=openvino-2021.1.0
xbuild_image:Custom Mac=openvino-2021.1.0

test_modules:Custom=gapi
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

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

OrestChura · 2021-03-11T15:13:21Z

@aDanPin please review

mpashchenkov

First wave. Superficially.

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

modules/gapi/test/common/gapi_video_tests_common.hpp

OrestChura · 2021-03-12T09:46:57Z

@anna-khakimova please take a look

modules/gapi/test/common/gapi_video_tests_common.hpp

modules/gapi/perf/common/gapi_video_perf_tests.hpp

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

mpashchenkov

P2

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

modules/gapi/test/common/gapi_video_tests_common.hpp

anna-khakimova · 2021-03-18T12:04:03Z

Looks good. Approved 👍

TolyaTalamanov · 2021-03-18T12:05:58Z

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

+PERF_TEST_P_(KalmanFilterControlPerfTest, TestPerformance)
+{
+    MatType2 type = -1;
+    int  dDim = -1, mDim = -1;


double space

You're right, thanks!
Fixed

…esting added

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

mpashchenkov

Looks good for me. Thanks.

OrestChura · 2021-03-24T06:17:50Z

@alalek I believe on our side we are ready to merge this

alalek · 2021-03-24T06:53:19Z

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

+                                std::vector<cv::Mat>& measurements)
+{
+    cv::RNG& rng = cv::theRNG();
+    measurements = std::vector<cv::Mat>(testNumMeasurements, cv::Mat::zeros(mDim, 1, type));


This statement looks wrong and must be avoided.
cv::Mat are not cloned here. All of them points to the same .data.
So initialization below through randu() just rewrites data in all affected containers.

Yes, thank you! A bad mistake
Fixed

alalek · 2021-03-24T06:53:54Z

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

+    cv::RNG& rng = cv::theRNG();
+    measurements = std::vector<cv::Mat>(testNumMeasurements, cv::Mat::zeros(mDim, 1, type));
+    haveMeasurements = std::vector<bool>(testNumMeasurements, true);
+    for (std::size_t i = 0; i < testNumMeasurements; i++)


std::size_t

Just size_t

OK
May I ask, is it an OpenCV convention, or it's just about this particular case for consistency of a function/file?
In any case, I perhaps can search through gapi-code to fix inconsistencies in a separate PR

They are both defined in OpenCV scope and have the same meaning.

Function parameter declared as size_t testNumMeasurements, need to be consistent.

OK, got it
Fixed

alalek · 2021-03-24T07:00:39Z

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

+    MatType2 type = -1;
+    int dDim = -1, mDim = -1;
+    std::size_t testNumMeasurements = 0;
+    bool receiveRandMeas = true;
+    cv::GCompileArgs compileArgs;
+    std::tie(type, dDim, mDim, testNumMeasurements, receiveRandMeas, compileArgs) = GetParam();


BTW, This approach doesn't make code cleaner.
Just brings unnecessary pre-initialization with -1, 0, true, etc

const auto& param = GetParam(); MatType2/*or auto*/ type = get<0>(param); const auto dDim = get<1>(param); const auto mDim = get<1>(param); ...

/cc @dmatveev

Hm, I agree it's inconvenient about pre-initialization, and variables aren't const.
But std::tie is more "secure" in a way that you can be sure you've got all the data from GetParam and haven't forgotten anything. In case of changing the parameters (adding ones at the end) there will be no errors. Something like that @dmatveev has explained me some time ago

alalek · 2021-03-24T07:16:02Z

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp

+    ctrls = std::vector<cv::Mat>(testNumMeasurements, cv::Mat(cDim, 1, type));
+    for (std::size_t i = 0; i < testNumMeasurements; i++)
+    {
+        cv::randu(ctrls[i], cv::Scalar::all(-1), cv::Scalar::all(1));


[G-API]: Performance tests for KalmanFilter * Kalman perf.tests and some tests refactoring * Input generation moved to a separate function; Slowest case sneario testing added * Generating refactored * Generating refactoring * Addressing comments

OrestChura added test category: g-api / gapi labels Mar 11, 2021

OrestChura requested a review from mpashchenkov March 11, 2021 15:15

mpashchenkov changed the title ~~[G-API]: Performance tests for KalmenFilter~~ [G-API]: Performance tests for KalmanFilter Mar 11, 2021

mpashchenkov reviewed Mar 11, 2021

View reviewed changes

OrestChura force-pushed the oc/Kalm_ptest branch 2 times, most recently from c384aab to 9f43032 Compare March 12, 2021 13:18

anna-khakimova reviewed Mar 15, 2021

View reviewed changes

modules/gapi/test/common/gapi_video_tests_common.hpp Outdated Show resolved Hide resolved

aDanPin reviewed Mar 15, 2021

View reviewed changes

modules/gapi/perf/common/gapi_video_perf_tests.hpp Show resolved Hide resolved

modules/gapi/perf/common/gapi_video_perf_tests.hpp Outdated Show resolved Hide resolved

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp Outdated Show resolved Hide resolved

mpashchenkov reviewed Mar 15, 2021

View reviewed changes

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp Outdated Show resolved Hide resolved

modules/gapi/test/common/gapi_video_tests_common.hpp Outdated Show resolved Hide resolved

OrestChura requested a review from mpashchenkov March 18, 2021 10:09

aDanPin approved these changes Mar 18, 2021

View reviewed changes

TolyaTalamanov reviewed Mar 18, 2021

View reviewed changes

OrestChura added 2 commits March 19, 2021 21:08

Kalman perf.tests and some tests refactoring

7792de6

Input generation moved to a separate function; Slowest case sneario t…

0f86395

…esting added

OrestChura force-pushed the oc/Kalm_ptest branch from a7d56ea to 0f86395 Compare March 19, 2021 18:18

mpashchenkov reviewed Mar 22, 2021

View reviewed changes

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp Show resolved Hide resolved

Generating refactored

3f60118

mpashchenkov reviewed Mar 22, 2021

View reviewed changes

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp Show resolved Hide resolved

modules/gapi/perf/common/gapi_video_perf_tests_inl.hpp Show resolved Hide resolved

Generating refactoring

ab114b0

mpashchenkov approved these changes Mar 22, 2021

View reviewed changes

OrestChura assigned mpashchenkov Mar 22, 2021

alalek reviewed Mar 24, 2021

View reviewed changes

Addressing comments

d9c9cdc

alalek merged commit ad2f5cc into opencv:master Mar 26, 2021

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged

Uh oh!

Conversation

OrestChura commented Mar 11, 2021

Pull Request Readiness Checklist

Uh oh!

OrestChura commented Mar 11, 2021

Uh oh!

mpashchenkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OrestChura commented Mar 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpashchenkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anna-khakimova commented Mar 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpashchenkov left a comment

Choose a reason for hiding this comment

Uh oh!

OrestChura commented Mar 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants