Unit tests for featur detector
ClosedPublic
Actions

Authored by Sergey Sharybin (sergey) on Feb 14 2014, 9:15 AM.

Details

Reviewers

Commits

rLMV80d6945bf5f9: Unit tests for feature detector

Summary

Currently covers only simplest cases with synthetic images.
Also at this point mainly Harris detector is being testes,
other detectors behaves a bit unexpected on synthetic images
and this is to be investigated further.

Tests will be extended further later.

Additional change:

Added constructor to Feature structure
Added operator << for feature for easier debug dumps.

Diff Detail

Repository: rLMV Libmv
Branch: detector_unittests

Event Timeline

Nice work adding tests!

src/libmv/simple_pipeline/detect.cc
148	Hmm; zero size? Perhaps add a constructor for zero sized as well. Also, there is an implied size for harris depending on how big the blur kernel for the derivative is. I think it's on the order of 5x5 in this case.
252–260	I think the cast to float is unneeded, since the +8.0f will do that already. Also, the size was 16 before but now is zero; is that intended? Also why 8.0? Constant needs comment. I know these issues existed before but now I'm looking at it more carefully.
307–310	See above comments. These do have an implied scale.
328	Can you make this a constant at the top of the file with a TODO? It'd be nice to make all the "needs discussion" constants at the top. I don't feel strongly about this though.
src/libmv/simple_pipeline/detect.h
37–38	This probably needs more explanation; is it in pixels? Is it a square size? is it a radius or the whole size? perhaps this should give an example of "if the feature is approximately a 5x5 square region, then size should be 5".
src/libmv/simple_pipeline/detect_test.cc
144	Commented out code banned; either fix the tests or remove them.
169	Fix or remove.

Replied some questions. Will update patch later -- currently we'll be busy with the release candidate bizz..

src/libmv/simple_pipeline/detect.cc
148	What is the of FAST feature? Is it 9 pixels as for the kernel size?
252–260	Depends. With strict conversion flags in gcc you'll have warning. That's probably not so much harmful tho. Do we need this to be changed? I don't see where size is 0 actually. 8 because of feature size is 16. SAD above calculates the score starting from the top left corner of the pattern window, and adding 8 would give you middle of the window. This indeed old issues and needed to be solved ages ago. But ok, will do it as a part of this patch.
307–310	Will do if you think we really need size and score.
328	Don't mind moving this to the top.
src/libmv/simple_pipeline/detect.h
37–38	This is from Mathias, could only guess what exactly this means. And this looks indeed to be a size of the feature in pixels. But there're several issues with this: Why it's float value? Currently it's only set by Moravec detector. No idea how to get feature size for other detectors. I'm not sure it's nice idea to use the same size for marker pattern as the feature size. Pattern is to be bigger than the feature anyway i guess. And it still doesn't solve issues with search window size. This size is never used actually. So i wouldn't cry if this thing is gone. Same goes to the score. This thing is totally unmeasurable, depends on detector type and i don't know who one would want to have it exposed?
src/libmv/simple_pipeline/detect_test.cc
144	Eeeek. I know it's banned. but it is here for a reason ;) Do you see a question in the comment? All this commented tests are here for discussion with you actually. They showed cases when detectors doesn't work as i was expecting them to do and not sure whether it's a limitation of the detectors, or it's a bug somewhere or so.

Addressed most of the issues. Only remained address commented tests, but let's think of them a bit more. It might be either specific work of detectors (as in malformed tests) or it might be a bug somewhere..

Patch will arrive soon :)

src/libmv/simple_pipeline/detect.cc
148	Used 9 as the feature size now.
252–260	Commented why its 8 and 16. But will think of having explicit float casts to avid possible warnings caused by paranoid compiler flags (which might be helpful in other areas).
307–310	Made it 5 pixels now. Seems to be correct.
328	Done. Also mofed FAST min_trackness to the top. They both are to be investigated for a better default value.
src/libmv/simple_pipeline/detect.h
37–38	Reshuffled comments. Didn't really like duplicated comments above the structure and inside the structure.
src/libmv/simple_pipeline/detect_test.cc
144	Can not do anything about this yet. Need your brains to check whether those are malformed tests or whether there are some issues in the detectors.
169	Same as above.

Address most of the feedback.

Keir Mierle (keir) accepted this revision.Mar 17 2014, 5:45 PM

I am not sure about FAST and MORAVEC. Do they have reference implementations that you could check against?

src/libmv/simple_pipeline/detect_test.cc
66	considered
89	Should check that the vector lengths match
92	There could be extra features that are not expected; is this what you mean here?
131	Is this closing the namespace? Add a // namespace ...

Address review from @Keir Mierle (keir)

Didn't really see any reference tests in FAST sources and didn't see reference MORAVEC implementation at all :S

src/libmv/simple_pipeline/detect_test.cc
66	Done.
89	It's checked two lines above. But added a return statement if length doesn't match so iteration doesn't happen if lengths are different.
92	No. Vectors should totally match. Since length are guaranteed to match at this point think this check actually means "two vectors are identical (only order of features might be different)"?
131	Done. Forgot to do this :S