Fix/vlfeat compliance by griwodz · Pull Request #112 · alicevision/popsift

griwodz · 2020-10-05T19:57:34Z

Description

The proposed change adds a descriptor computation that is compliant with VLFeat. The original PopSift descriptor was not because it used the angle differently: while VLFeat spreads the angular part of the descriptor over up to 8 bins of the descriptor's histogram, PopSift would only spread it over two. Furthermore, descriptors are arranged differently into the histograms (according to VLFeat documentation, VLFeat does that also differently from Lowe's original code).

Features list

For backward compatibility, the VLFeat descriptor is not the default descriptor. The following settings lead to results that seem to be identical to the VLFeat 0.9.20 C code and command line (the 0.9.21 command line does not work):

--desc-mode=vlfeat              : use the VLFeat descriptor extraction
--norm-mode=classic             : use L2 normalization, not RootSift
--norm-multi 9 --write-as-uchar : multiply descriptor values with 2^9 and cast to uchar
--write-with-ori                : output of descriptors in the same format as VLFeat command line (instead of OpenCV-compliant)
--initial-blur 0.5              : PopSift default
--sigma 1.6                     : PopSift default
--threshold 0                   : set the peak threshold to 0
--edge-threshold 10.0           : PopSift default

Implementation remarks

The L2 normalization had a bug that was removed.

The orientation computation does now use a bilateral filter, which does not appear to have any visible effect. This is a change from the previous PopSift default, and it may be safer to make it a VLFeat-specific option.

The threshold for accepting an orientation in VLFeat is computed before smoothing the histogram, PopSift did it after computing the histogram (consequently accepting fewer orientations). This was change to work like VLFeat.

griwodz · 2020-10-06T12:44:43Z

    }
    norm = popsift::shuffle( norm,  0 );

-    descr.x = min( descr.x, 0.2f*norm );


I made a direct comparison between the develop branch and develop with the L2 normalization bug fixed.
I compare the descriptors extracted from all images in the dataset containing boat, bark, bikes, graf, leuven, tree, ubc and wall.
The average number of descriptors extracted per image was 10000.
The largest difference before and after the fix was 2 descriptors changed in 1 value.

simogasp · 2020-10-06T14:20:43Z

+        "loop, iloop, grid, igrid, notile, vlfeat\n"
 	"Default is loop\n"
-        "loop is OpenCV-like horizontal scanning, computing only valid points, grid extracts only useful points but rounds them, iloop uses linear texture and rotated gradiant fetching. igrid is grid with linear interpolation. notile is like igrid but avoids redundant gradiant fetching.")
+        "loop is OpenCV-like horizontal scanning, computing only valid points, grid extracts only useful points but rounds them, iloop uses linear texture and rotated gradiant fetching. igrid is grid with linear interpolation. notile is like igrid but avoids redundant gradiant fetching, vlfeat replicates vlfeat behaviour.")


maybe briefly remind the vlfeat behaviour like done for opencv for example

I have extended it and move it to Config::getDescModeUsage(). The usage is getting very long, I wonder if we should re-write this to support --desc-mode=?

simogasp · 2020-10-06T14:27:34Z

        /// variant of IGrid, no duplicate gradient fetching
-        NoTile
+        NoTile,
+        /// extraction code according to VLFeat


same here, maybe briefly remind the difference so we have it in the doc

simogasp · 2020-10-06T14:41:04Z

 /*
 * Histogram smoothing helper
 */
+#ifdef WITH_VLFEAT_SMOOTHING


it would be better to use POPSIFT_WITH_VLFEAT_SMOOTHING or POPSIFT_USE_VLFEAT_SMOOTHING to avoid conflict and for consistency through repositories.

Also, is this supposed to be an option the user can choose or is it gonna be a kind of debugging or dev-only feature?
In the first case, we should then also add the option in cmake and add the definition at compile time

option(PopSift_USE_VLFEAT_SMOOTHING "Use VLFeat smoothing." ON) ... # set the variable if(PopSift_USE_VLFEAT_SMOOTHING) set(POPSIFT_USE_VLFEAT_SMOOTHING 1) else() set(POPSIFT_USE_VLFEAT_SMOOTHING 0) endif() ... # add also the message in the final summary message("******************************************") message("Building configuration:\n") message(STATUS "PopSift version: " ${PROJECT_VERSION}) message(STATUS "Build type: " ${CMAKE_BUILD_TYPE}) ... message(STATUS "Using VLFeat smoothing: " ${POPSIFT_USE_VLFEAT_SMOOTHING})

and add the entry in cmake/sift_config.h

#define POPSIFT_USE_VLFEAT_SMOOTHING() @POPSIFT_USE_VLFEAT_SMOOTHING@

and #include <popsift/sift_config.h> wherever it is used

I think we can remove the alternative and keep only VLFEAT_SMOOTHING. This is from the original development and the other setting has never been used.

simogasp · 2020-10-06T14:41:43Z

+/* Bilinear histogram seems to be VLFeat's default now.
+ * We emulate it because we have different orientations without it.
+ */
+#define WITH_BILINEAR_ORI


same comment as WITH_VLFEAT_SMOOTHING

I'm just comparing the effect of every little change on the descriptors. I think that the difference is big enough to make WITH_BILINEAR_ORI a Config parameter instead of a define.

simogasp · 2020-10-06T14:43:13Z

-        bool predicate = ( bin < ORI_NBINS ) && ( sm_hist[bin] > max( sm_hist[prev], sm_hist[next] ) );
+        bool predicate = ( bin < ORI_NBINS ) && ( sm_hist[bin] > max( sm_hist[prev], sm_hist[next] ) ) && (sm_hist[bin] > 0.8f * maxval);

+#define REFINE 2


maybe we can use WITH_VLFEAT_SMOOTHING or WITH_BILINEAR_ORI?

I have removed WITH_VLFEAT_SMOOTHING because we have used only "true" for years.
I have replaced WITH_BILINEAR_ORI with a command line switch --ori-mode=BestBin or InterpolatedBin. The first is original PopSift behaviour, the second current VLFeat behaviour. But note that VLFeat is also using a #define to support BestBin instead.

…tion

BestBin is the original PopSift approach. InterpolatedBin is a mode that emulates VLFeat behaviour.

[doc] Integrating Config defaults into the Usage. Co-authored-by: Simone Gasparini <simone.gasparini@gmail.com>

griwodz · 2020-10-08T15:40:55Z

Seeing the descriptor distances, I have to go back to the code and check if there's a place in the code where I'm swapping the flags, because this is weird.

griwodz · 2020-10-11T18:15:51Z

@simogasp @fabiencastan Now it is really time to compare the VLFeat-compatible descriptor with the VLFeat-trained voctree. Our original orientation computation was never the problem, the descriptor computation is different. This led to a different probability distribution of values in the descriptors' 128 floats, which explains why the voctree code couldn't find matches with PopSift.

griwodz · 2020-10-11T18:28:14Z

This is the difference in the individual cells of the old PopSift descriptor. The 128 values are 4x4 histograms of 8 rotational bins each. The figure shows the 8 bins sides by side, for each bin the average difference between VLFeat and PopSift descritors for a 4x4 group.

You can see that the PopSift descriptor values are generally higher in the first bin, lower in the later bins.

griwodz · 2020-10-11T18:28:42Z

Now, with the VLFeat-compatible descriptor, it looks like this:

The distribution is now pretty much identical.

griwodz · 2020-10-11T18:37:23Z

The success for correct matching is also quite a bit better. The figure shows L2 distance between the best descriptors (in purple) between the old descriptor on the left and VLFeat-compatible descriptor in the right.

simogasp · 2020-10-11T21:18:08Z


+static bool stringIsame( string l, string r )
+{
+    std::for_each( l.begin(), l.end(), [](char& c) { c = ::tolower(c); });


Suggested change

std::for_each( l.begin(), l.end(), [](char& c) { c = ::tolower(c); });

// early exit

if(l.size() != r.size())

{

return false;

}

std::for_each( l.begin(), l.end(), [](char& c) { c = ::tolower(c); });

I take the proposed change. I wanted to go for strncmp, but Windows does apparently not have it, it has stricmp. And adding Boost just for this seemed like too much.

@simogasp It would be really preferable to use strcasecmp() for non-Windows and _stricmp() for Windows. The first requires inclusion of strings.h (at least on Linux and Mac) and the other string.h. The first one is a function, the second is a macro. I'm too stupid to write a CMake test, so I gave up and wrote these crappy lines to make it run.
And is speed essential? It should be used to evaluate the command line.

I think it is ok like this, it is just for comparing the string in the enum. And I agree that it's not worth to re-add boost just for this.

Co-authored-by: Simone Gasparini <simone.gasparini@gmail.com>

griwodz · 2020-10-12T08:59:35Z

Btw.: the nice results were created with the following command line:
popsift-demo --pgmread-loading --norm-mode=classic --norm-multi 9 --write-as-uchar --write-with-ori --initial-blur 0.5 --sigma 1.6 --threshold 0 --edge-threshold 10.0 --desc-mode=vlfeat -i boat.pgm

Carsten Griwodz added 11 commits October 5, 2020 12:17

[popsift] choosing a runtime between two descriptor printing mode

1e4d1df

[bugfix] vlfeat compliance in orientation computation

fa695d6

Merge branch 'develop' into fix/misalignBugOri

608eac5

[demo] print default values in usage

0b8fb18

[bug] corrected truncating in L2 normalization

92f1846

[bug] added reverse square root required by previous bugfix

a529cad

[feature] VLFeat-compliant descriptor extraction

a9ca694

[popsift] cleanup

003e7ea

[popsift] first parallelism in vlfeat descriptors

52c3cbe

[bugfix] limit accepted orientations to histogram entries 0.8*max again

5c0256f

[popsift] use 32 threads for each descriptor

bf59959

griwodz requested review from fabiencastan, lcalvet and simogasp October 5, 2020 19:58

griwodz self-assigned this Oct 5, 2020

griwodz added bugfix cuda issues related to cuda versions feature request prio:critical labels Oct 5, 2020

griwodz added this to the v1.0.0 milestone Oct 5, 2020

[popsift] commented extended option

d93b8e3

griwodz commented Oct 6, 2020

View reviewed changes

simogasp reviewed Oct 6, 2020

View reviewed changes

Carsten Griwodz added 7 commits October 6, 2020 17:31

[popsift] avoid a conflict between old and vlfeat orientation computa…

9349547

…tion

[cleanup] remove histogram smoothing mode that was never used

942aea1

[cleanup] remove unused code

aea885e

[popsift] move rotation compensation to descriptor extraction

b7bbcd2

Merge branch 'develop' into fix/vlfeatCompliance

b3f8434

[popsift] choose between orientation computation modes

e9ec403

BestBin is the original PopSift approach. InterpolatedBin is a mode that emulates VLFeat behaviour.

[bugfix] fix half-bin shift in Interpolated mode

cbf70d0

Carsten Griwodz and others added 4 commits October 8, 2020 12:37

[popsift] minor range modification in orientation computation

2708e89

[doc] improved doc for enum DescMode

bbc0e75

[doc] moved the Usage for the DescMode parameter to sift_conf.cu

1ea511e

[doc] Integrating Config defaults into the Usage.

6eeaee3

[doc] Integrating Config defaults into the Usage. Co-authored-by: Simone Gasparini <simone.gasparini@gmail.com>

Carsten Griwodz added 4 commits October 8, 2020 19:21

[popsift] added Config::getSigma()

e370180

[popsift] rotate orientation by 180deg in all older modes

5591c5b

[test] draft comparison code

6faa3f4

[popsift] remove circular limitation for descriptor in VLFeat mode

3cdc329

Carsten Griwodz added 3 commits October 11, 2020 23:06

[popsift] avoid boost in libpopsift

59a70c0

[test] updating descriptor file comparison

1a54748

[testing] remove test file, prioritize main PR

085694b

simogasp reviewed Oct 11, 2020

View reviewed changes

Comment thread src/popsift/sift_conf.cu Outdated

simogasp reviewed Oct 11, 2020

View reviewed changes

griwodz and others added 2 commits October 12, 2020 07:03

[popsift] shorter replace strcmp with string::op==

3673a85

Co-authored-by: Simone Gasparini <simone.gasparini@gmail.com>

[popsift] add more Config::get* functions, improve usage

bd1c8ef

[popsift] remove failed interpolating orientation code

74f7053

griwodz removed cuda issues related to cuda versions feature request prio:critical labels Oct 16, 2020

griwodz mentioned this pull request Oct 16, 2020

VLFeat compatibility #114

Closed

griwodz closed this Jan 5, 2021

-    std::for_each( l.begin(), l.end(), [](char& c) { c = ::tolower(c); });
+    // early exit
+    if(l.size() != r.size())
+    {
+        return false;
+    }
+    std::for_each( l.begin(), l.end(), [](char& c) { c = ::tolower(c); });

Conversation

griwodz commented Oct 5, 2020

Description

Features list

Implementation remarks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

griwodz commented Oct 8, 2020

Uh oh!

griwodz commented Oct 11, 2020

Uh oh!

griwodz commented Oct 11, 2020

Uh oh!

griwodz commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

griwodz commented Oct 11, 2020

Uh oh!

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!

griwodz commented Oct 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

griwodz commented Oct 11, 2020 •

edited

Loading