Adding the LATCH descriptor #231

Merged
merged 11 commits into from May 23, 2015

Projects

None yet

5 participants

@GilLevi
GilLevi commented May 13, 2015

Based on the following paper:
Gil Levi and Tal Hassner, "LATCH: Learned Arrangements of Three Patch Codes", arXiv preprint arXiv:1501.03719, 15 Jan. 2015

link:
http://arxiv.org/abs/1501.03719

Note: this code is submitted to the OpenCV CVPR15 challenge in the category of local image descriptors (specifically, binary descriptors) - the Affine Covariant Regions Datasets.

OpenCV challenge page:
http://opencv.org/opencv-vision-challenge.html

Affine Covariant Regions Datasets:
http://www.robots.ox.ac.uk/~vgg/research/affine/

Gil.

@adrians
adrians commented May 13, 2015

Hi Gil,

Great contribution!
After building without errors, you should also squash the patches into a single one (see the git rebase command) to be simpler to manage.

After that, a member of the project will look over and review the modification.

-- Adrian

@GilLevi
GilLevi commented May 14, 2015

@adrians - Thanks for you comment. Can you please elaborate? I didn't understand what the specific problem is and how to solve it.

Thanks!

@adrians
adrians commented May 14, 2015

@GilLevi - Sure. As a measure to keep the history of the git repository clean, a pull request should contain only one patch.

The operation to merge commits is called squashing. From the command line you should do git rebase -i HEAD~9 and mark all the fixes as "fixup commits". This will "rewrite the history" as if you wrote a good commit from the start. more info here. After this, a git push -f should also update this pull request.

@cbalint13
Contributor

@GilLevi , Congratulations for this work ! Very nice one, i believe is the very first "trained" descriptor addition here. I am eager to try this out asap !


I believe this below would be only optional, but:

Would it be possible to add some QA tests too for this PR ? I blieve it could be only optional. By QA I mean to add it to: modules/xfeatures2d/test/test_rotation_and_scale_invariance.cpp , perform some perf-tests and adjust those values as high as it just descriptor would pass the tests. This way a QA would be assured over time and also the threshold values idicates the quality of descriptor.

But i think this PR could pass without QA step. If someone would like it in the feature can do another PR just for QA part.

@cbalint13
Contributor

@GilLevi ,

  • Permit to ask you if there are plans to release the trainer part of this descriptor too ? I myself would love to see that part too. It could allow people to try out much better solutions or ideas over the trainer part, perhaps other datasets, deeper or much prolonged trainig steps.

This question can be a bit off the topic here but i am just curious, apologies for that.

@vpisarev
Contributor

@GilLevi, thank you for the contribution! Look great! May I ask you to correct indentation though? It's 6 spaces somewhere, or 3 spaces or 4. It can be done semi-automatically if you copy-n-paste the source code to some good editor like Emacs, Xcode, TextMate, Visual Studio etc. If possible, set 4 spaces as indentation size before doing that.

@vpisarev vpisarev commented on the diff May 16, 2015
modules/xfeatures2d/include/opencv2/xfeatures2d.hpp
@@ -145,6 +145,21 @@ class CV_EXPORTS LUCID : public DescriptorExtractor
};
+/*
+* LATCH Descriptor
+*/
+
+/** latch Class for computing the LATCH descriptor.
+If you find this code useful, please add a reference to the following paper in your work:
+Gil Levi and Tal Hassner, "LATCH: Learned Arrangements of Three Patch Codes", arXiv preprint arXiv:1501.03719, 15 Jan. 2015
@vpisarev
vpisarev May 16, 2015 Contributor

can you also add some descriptions for parameters and the algorithm itself. E.g. that this is binary descriptor extractor, that it's recommended to use it with FAST or AGAST or whatever

@vpisarev
Contributor

@GilLevi, oh, I actually do not see any sample or regression test. Could you please add some?

@GilLevi
GilLevi commented May 16, 2015

Thanks for the comments and tips !

@cbalint13 , @vpisarev - I've added a sample of LATCH in the following pull request:
opencv/opencv#4015

Can you please tell me which other tests do I need to add?

@vpisarev - I've added a short description of the algorithm and a few words regarding parameters. I've also fixed the indentation. Any comments on those changes?

@cbalint13 - the learning was implemented in Matlab. However, I might add a different technique of learning that will be implemented in C++. If so, then I will gladly add it to the commit !

@adrians - Thanks for the explanation. I will fix it.

Finally, I've seen that in other descriptors documentation, they use the "@" sign. For example:
@brief Class implementing the locally uniform comparison image descriptor, described in @cite LUCID

I assume it's related to automatically generated documentation. Can someone please elaborate? should I add it for LATCH ?

Also, any other tips on how to make this PR better?

Thanks again for all the comments and tips!
Gil.

@cbalint13
Contributor

@GilLevi ,

  • Regression and Performance test should be added to perf/ and test/ folder. I think its mandatory part in opencv to have those, its quite easy to add them. I described for you in an earlier post right here (upper) with links to some examples how to do it, there was a reference to rotation invariance test for BRIEF too (your work).
  • If training code is in matlab i (at least one myself) would love to transcribe if you made it available somewhere in the public.
@GilLevi
GilLevi commented May 18, 2015

@cbalint13 - Thanks for your help.

I've tried to add LATCH to test_features2d.cpp. However, the test did not pass since LATCH deletes keypoints that are too close to the image border, hence, the following test fails (line 412):

        if( calcDescriptors.rows != (int)keypoints.size() )
        {
            ts->printf( cvtest::TS::LOG, "Count of computed descriptors and keypoints count must be equal.\n" );
            ts->printf( cvtest::TS::LOG, "Count of keypoints is            %d.\n", (int)keypoints.size() );
            ts->printf( cvtest::TS::LOG, "Count of computed descriptors is %d.\n", calcDescriptors.rows );
            ts->set_failed_test_info( cvtest::TS::FAIL_INVALID_OUTPUT );
            return;
        }

Any tips on how to resolve it?

Thanks!
Gil

@cbalint13
Contributor

@GilLevi ,

1 See some diff's here for you, it might help: http://openrisc.rdsor.ro/latch-for-gil/

  • It failed due to improper things in compute() routine (see: latch.cpp.diff).
    • I made for you all the tests, it pass all fine ! {see: perf.latch.cpp, test_features2d.cpp.diff,test_rotation_and_scale_invariance.cpp.diff}
  • Rotation invariance test is quite nice one it pass at 97% ! {curr = 0.970874}
  • Scale invariance tests are skipped, its not the case for LATCH (it fail, but i think is expected).

2 Could please rename the whole class LATCHDescriptorExtractor into LATCH ? It is more simple and usable just like LUCID, SURF, BRISK, SIFT , etc are. The original name is very long, and the "DescriptorExtractor" suffix is not in practice anymore at least since opencv 3.0.

Best,
~christian.

@GilLevi
GilLevi commented May 20, 2015

@cbalint13 - Thanks for your huge help! I just pushed some modifications and added the tests.

Gil.

@vpisarev vpisarev self-assigned this May 23, 2015
@vpisarev
Contributor

👍

@vpisarev vpisarev merged commit 7b6fe5c into opencv:master May 23, 2015

1 check passed

default Required builds passed
Details
@urimerhav

Impressive stuff Gil! You rock!

@cbalint13
Contributor

@GilLevi ,

  • Would you mind to release the trainer part in matlab somewhere ? I would help to transcribe it for opencv.

With Respect,
~christian.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment