goodFeaturesToTrack returns also corner value by WisdomPill · Pull Request #17836 · opencv/opencv

WisdomPill · 2020-07-14T06:30:30Z

This is still a work in progress, for the moment I have been working only on the function goodFeaturesToTrack and not the GFTT Detector. This is also my first time with c++ since university, please bear with me if you see naive errors.

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 OpenCV (BSD) 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

I have some issue with perf tests, I can't understand why I get this error.

/Users/wisdompill/Projects/opencv/modules/ts/src/ts_perf.cpp:470: Failure
Expected equality of these values:
  expected_type
    Which is: 13
  array.type()
    Which is: 21
  Argument "corners" has unexpected type

I changed the types accordingly but I cannot understand why is this happening.

I removed the test_cornerEigenValsVecs function because it was incomplete resulting in bad accuracy and I think not needed for testing goodFeaturesToTrack

force_builders=linux,docs

WisdomPill · 2020-07-16T06:45:17Z

@asmorkalov @alalek can I close this and reopen differently? I forgot to create a branch for this PR.

vpisarev · 2020-07-17T10:52:24Z

@WisdomPill, thank you for the contribution!

I suggest to make the patch more compatible with the existing API.

cvGoodFeaturesToTrack is C API. It's frozen and will be completely removed in OpenCV 5.0 (due to be out in the end of 2020). So, please, do not change it.
Please, add another output parameter, OutputArray cornerValues=noArray() to cv::goodFeaturesToTrack. It will take CV_32F matrix or std::vector. There you can store corner values without touching keypoints themselves.
The newly added KeyPoint(Point3f, ...) should also be eliminated. Use the existing KeyPoint constructor, where you can pass 2D point and the computed corner value from a separate vector.

WisdomPill · 2020-07-17T11:01:02Z

Okay, got it, I will do it in the beginning of next week, do you think I should create another branch?

I already did here, and probably will do the same in opencv/opencv_extra

asmorkalov · 2020-07-20T05:43:57Z

@WisdomPill no need to create another branch and PR. I propose to continue work here.

… compatibility

WisdomPill · 2020-07-22T09:21:14Z

Should be okay by now, I also update opencv_extra for the perftests.

I will tackle the cuda version of the GFTTDetector I might need more time for this, I saw that the cuda version does not implement feature2d methods and also does not provie getters and setters.

I pushed opencv_extra on the branch gftt_response_value, I mentioned this pull request on the pull request of opencv_extra

mshabunin · 2020-07-22T09:41:21Z

@WisdomPill , we try to avoid increasing sanity checks in performance tests. It would be better to make good accuracy test.

Your PR to opencv_extra have different branch name and will not be tested on our CI together with this one.

WisdomPill · 2020-07-22T09:51:12Z

Okay, I see, so your saying that corners response values should not be sanity checked in perf_tests but only the values in the accuracy in the unittests, for example here modules/imgproc/test/test_goodfeaturetotrack.cpp?

Also, I think that GFTTDetector needs testing, should I add it?

In this case the pull request for opencv_extra is not needed at all and the tests should pass in the next commit that fixes perf_tests.

Ibrarkhan55 · 2020-07-22T10:03:49Z

16642

mshabunin · 2020-07-22T10:38:06Z

corners response values should not be sanity checked in perf_tests

@WisdomPill , yes, exactly.

Also, I think that GFTTDetector needs testing, should I add it?

There are two tests in features2d module, perhaps they could be extended:

opencv/modules/features2d/test/test_keypoints.cpp

Lines 145 to 151 in b698d0a

    
           TEST(Features2d_Detector_Keypoints_GFTT, validation) 
        
           { 
        
               Ptr<GFTTDetector> gftt = GFTTDetector::create(); 
        
               gftt->setHarrisDetector(true); 
        
               CV_FeatureDetectorKeypointsTest test(gftt); 
        
               test.safe_run(); 
        
           }

opencv/modules/features2d/test/test_detectors_regression.cpp

Lines 45 to 49 in b698d0a

    
           TEST( Features2d_Detector_GFTT, regression ) 
        
           { 
        
               CV_FeatureDetectorTest test( "detector-gftt", GFTTDetector::create() ); 
        
               test.safe_run(); 
        
           }

WisdomPill · 2020-07-22T12:29:46Z

Okay, so as I understand the tool is always the same modules/ts/misc/run.py but there is an accuracy flag,
maybe the wiki has to be updated a little bit. I had a look at this https://github.com/opencv/opencv/wiki/How_to_contribute

Should I push the accuracy tests on master branch as well in opencv_extra?
So that the continuous integration bot can pick it up, otherwise a new branch is needed.

mshabunin · 2020-07-24T10:56:36Z

@WisdomPill , yes, in order to test changes together branches should have the same name.

You can run tests directly without run.py, e.g. ./bin/opencv_test_imgproc --gtest_filter=*GFTT*. Some more information about tests can be found here: https://github.com/opencv/opencv/wiki/QA_in_OpenCV

asmorkalov · 2020-08-03T12:55:45Z

@WisdomPill Do you have a chance to finish the PR in meantime?

WisdomPill · 2020-08-03T15:04:44Z

Yes sorry, I am on holiday, and will come back this weekend. I had finished most of the work, I was just missing the accuracy tests. Is it okay if I finish when I will be back?

…rs values

WisdomPill · 2020-08-10T09:27:41Z

I do not understand how to update the xml file for accuracy tests.

Accuracy tests seem to pass without changes, maybe it is because the function isSimilarKeyPoints here

opencv/modules/features2d/test/test_detectors_regression.impl.hpp

Line 66 in b698d0a

    
           bool CV_FeatureDetectorTest::isSimilarKeypoints( const KeyPoint& p1, const KeyPoint& p2 )

has a high maxResponseDif value. If I change it and lower it to 0.001 the test fail like it should but also SIFT fails.

Is this the file to update (https://github.com/opencv/opencv_extra/blob/master/testdata/cv/features2d/feature_detectors/detector-gftt.xml.gz)?
Can you please show me how to do that?

Also one of the perf tests is failing, and I do not understand one kind of error regarding the size of corners variable here https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/27302/steps/perf_imgproc/logs/tests%20summary

the other errors are about degradation in performance?

vpisarev · 2020-08-18T13:16:11Z

modules/imgproc/test/test_goodfeaturetotrack.cpp

    { return *a > *b; }
 };

-static void


why is the reference implementation removed? please, restore and use it

I removed it because I was also testing the response value which was not accurate with that test method.
I am restoring it right now

vpisarev · 2020-08-18T13:19:40Z

@WisdomPill, it looks like in this particular test it's enough to erase the existing detector-gftt.xml.gz and run the test, then it will re-create it.

Also, I as mentioned above, please, restore the reference implementation test_cornerEigenValsVecs. Otherwise, it will be a meaningless test – to compare the function output with itself.

WisdomPill · 2020-09-01T11:06:08Z

@vpisarev thank you for the hint, I updated detector-gftt.xml.gz by deleting the file and the test recreated it.
Can you help me understand why the accuracy on test_cornerEigenValsVecs is bad?
Should I keep testing for the accuracy there or not?

asmorkalov · 2020-11-24T19:38:14Z

@vpisarev @WisdomPill Friendly reminder.

vpisarev · 2020-12-01T23:21:53Z

@asmorkalov, @WisdomPill, I just run our buildbot — there are many problems. I guess, the changes in C API should be undone, because in C there is no optional arguments. Then, the test failures need to be fixed. In general, the patch can be merged, as long as buildbot is "green"

WisdomPill · 2020-12-03T08:35:03Z

@vpisarev I do not understand what changes are you referring to, I need more guidance because I am not used to c++. Can you help?

asmorkalov · 2021-02-03T09:21:07Z

Replaced by #19392. The new PR includes commits from the original PR and adds test fixes and sanity data.

WisdomPill · 2021-02-03T15:07:52Z

Thanks

goodFeaturesToTrack returns also corner value

4a8f067

WisdomPill changed the title ~~WIP goodFeaturesToTrack returns also corner value~~ goodFeaturesToTrack returns also corner value Jul 14, 2020

WisdomPill marked this pull request as ready for review July 14, 2020 07:24

asmorkalov added category: features2d RFC labels Jul 15, 2020

Added response to GFTT Detector keypoints

b88fb40

vpisarev self-assigned this Jul 17, 2020

asmorkalov added category: features2d and removed RFC category: features2d labels Jul 20, 2020

Moved corner values to another optional variable to preserve backward…

6137383

… compatibility

WisdomPill mentioned this pull request Jul 22, 2020

Updated perf tests for improc module opencv/opencv_extra#776

Closed

WisdomPill mentioned this pull request Jul 22, 2020

cuda::GoodFeaturesToTrackDetector does not have the same interface as the non cuda one opencv/opencv_contrib#2614

Open

3 tasks

WisdomPill added 2 commits August 10, 2020 09:21

Removed corners valus from perf tests and better unit tests for corne…

f3d0ef2

…rs values

Fixed detector gftt call

be29755

vpisarev reviewed Aug 18, 2020

View reviewed changes

Restored test_cornerEigenValsVecs

ea3e118

asmorkalov requested a review from vpisarev November 24, 2020 19:38

amirtu mentioned this pull request Jan 25, 2021

goodFeaturesToTrack returns also corner value #19392

Merged

6 tasks

asmorkalov closed this Feb 3, 2021

Uh oh!

Conversation

WisdomPill commented Jul 14, 2020 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

WisdomPill commented Jul 16, 2020

Uh oh!

vpisarev commented Jul 17, 2020

Uh oh!

WisdomPill commented Jul 17, 2020

Uh oh!

asmorkalov commented Jul 20, 2020

Uh oh!

WisdomPill commented Jul 22, 2020

Uh oh!

mshabunin commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WisdomPill commented Jul 22, 2020

Uh oh!

Ibrarkhan55 commented Jul 22, 2020

Uh oh!

mshabunin commented Jul 22, 2020

Uh oh!

WisdomPill commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mshabunin commented Jul 24, 2020

Uh oh!

asmorkalov commented Aug 3, 2020

Uh oh!

WisdomPill commented Aug 3, 2020

Uh oh!

WisdomPill commented Aug 10, 2020

Uh oh!

vpisarev Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

WisdomPill Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

vpisarev commented Aug 18, 2020

Uh oh!

WisdomPill commented Sep 1, 2020

Uh oh!

asmorkalov commented Nov 24, 2020

Uh oh!

vpisarev commented Dec 1, 2020

Uh oh!

WisdomPill commented Dec 3, 2020

Uh oh!

asmorkalov commented Feb 3, 2021

Uh oh!

WisdomPill commented Feb 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WisdomPill commented Jul 14, 2020 •

edited by alalek

Loading

mshabunin commented Jul 22, 2020 •

edited

Loading

WisdomPill commented Jul 22, 2020 •

edited

Loading