Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/vl feat compliance3 #133

Open
wants to merge 34 commits into
base: develop
Choose a base branch
from
Open

Fix/vl feat compliance3 #133

wants to merge 34 commits into from

Conversation

griwodz
Copy link
Member

@griwodz griwodz commented Jul 2, 2021

Description

This PR merges PRs #114, #115 and #128, making it easier for those who need it.

It adds two things:

  • faster grid filtering done on the CPU
  • results of brute-force GPU matching between two sets of descriptors, using the new method int3* FeaturesDev::matchAndReturn(FeaturesDev*), which are available both on the CPU and GPU side

From PR #114, it fixes a bug in L2 normalization.

From PR #114, it gets a descriptor computation that is compliant with VLFeat. The original PopSift descriptor was not compliant 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 the VLFeat documentation, VLFeat does that also differently from Lowe's original code and Changchang Wu's SiftGPU).

From PR #115, it gets a test script that runs VLFeat and PopSift on one or more images and creates the descriptors with orientations. compareSiftFiles.cpp performs a brute force CPU-sided comparison of the output files and outputs sorted files and some distance computations. These are then transformed with bash and awk and rendered visually with gnuplot.

From PR #128, it gets the ability to ignore Edge and Peak Thresholds. These are ignored when their respective configuration variables are set to 0.0f or less.

This PR adds grid filtering that does now rely on CUDA Managed Memory and sorts initial extrema on the CPU side. This is faster than the 2 former approaches that needed a lot of memory management to do it on the GPU side. Grid filtering makes it possible to overlay the image with a grid and ensure that similar numbers of extrema are used from each of the cells, up to an overall maximum. Grid filtering code is excluded if the CMake variabe PopSift_USE_GRID_FILTER is set to OFF.

Features list

  • Command line and Config settings to use VLFeat-compliant feature extraction.
  • Scripts that compare VLFeat-compatible PopSift behaviour with original VLFeat behaviour. A brute force CPU tool comparing two files containing SIFT descriptors. VLFeat and Matlab syntax supported (132 and 133 numbers per line, respectively).
  • Ignoring two checks when the respective Config values are 0 or less.
  • Added getDefault functions and usage descriptions to the Config class.

Implementation remarks

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 changed to work like VLFeat. This change is not restricted to modes that try to be as close to VLFeat as possible, but used for all modes.

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

@tkircher
Copy link

Adding #include <thrust/host_vector.h> to s_filtergrid.cu fixed the one and only compilation issue for me, just in case you were wondering if it was actually necessary for anyone.

@griwodz griwodz requested a review from simogasp January 25, 2022 12:42
@griwodz
Copy link
Member Author

griwodz commented Jan 25, 2022

@tkircher Thanks!
I'm pretty sure that this PR is a big improvement over develop. It may not be absolutely 100% identical to vlfeat yet, but the descriptors including the statistical distribution of their histograms look really similar now. There's also the minor normalization bug fixed and test code.

find_program(SORT sort)
find_program(GNUPLOT gnuplot)
find_program(CONVERT convert)
find_program(VLFEAT sift PATH_SUFFIXES glnxa64 bin/glnxa64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
find_program(VLFEAT sift PATH_SUFFIXES glnxa64 bin/glnxa64)
find_program(VLFEAT sift HINTS ${VLFEAT_BIN} PATH_SUFFIXES glnxa64 bin/glnxa64 REQUIRED)

It's better to have REQUIRED so it stops if it does not find it and ${VLFEAT_BIN} can be use as hint to find vlfeat

@@ -273,6 +232,8 @@ int main(int argc, char **argv)

std::cout << "PopSift version: " << POPSIFT_VERSION_STRING << std::endl;

config.setDescMode( popsift::Config::VLFeat_Desc );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a problem with your changes but it would be better to add an else branch in the check for the file to exist at line 246 below:

    if( boost::filesystem::exists( inputFile ) ) {
        if( boost::filesystem::is_directory( inputFile ) ) {
            cout << "BOOST " << inputFile << " is directory" << endl;
            collectFilenames( inputFiles, inputFile );
            if( inputFiles.empty() ) {
                cerr << "No files in directory, nothing to do" << endl;
                return EXIT_SUCCESS;
            }
        } else if( boost::filesystem::is_regular_file( inputFile ) ) {
            inputFiles.push_back( inputFile );
        } else {
            cout << "Input file is neither regular file nor directory, nothing to do" << endl;
            return EXIT_FAILURE;
        }
    }
+    else 
+    {
+        cout << "Input file/directory does not exist" << endl;
+        return EXIT_FAILURE;
+    }

so that the program exits if the input does not exist

# FILES="hash"
# FILES="boat1 boat"
# FILES="boat1"
FILES="boat"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting errors of files not found with this scrpt.
I think the error come from the ambiguity of FILES: here it seems that it contains the dataset(s) that we want to use, but almost everywhere in the script is used to name the file to process, e.g boat.pgm instead of boat/img1.pgm.
Is the script supposed to run on each and every image of (possibly) multiple datasets? If so I think it needs some work i.e. it misses a nested for img in img1 img2 img3.... almost everywhere

Comment on lines +68 to +80
for file in $1 ; do
if [ ! -f ${file}.pgm ]
then
${CMD_CONVERT} ${DIR_TEST}/$file/img1.ppm ${file}.pgm
if [ $? -ne 0 ]
then
echo " ${CMD_CONVERT} failed, test image not converted"
exit -1
else
echo " converted ${file} to PGM"
fi
fi
done
Copy link
Member

@simogasp simogasp Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, as an example here I think you need something more like this

Suggested change
for file in $1 ; do
if [ ! -f ${file}.pgm ]
then
${CMD_CONVERT} ${DIR_TEST}/$file/img1.ppm ${file}.pgm
if [ $? -ne 0 ]
then
echo " ${CMD_CONVERT} failed, test image not converted"
exit -1
else
echo " converted ${file} to PGM"
fi
fi
done
for file in $1 ; do
for img in img1 img2 img3 img4 img5 img6
do
if [ ! -f ${DIR_TEST}/$file/${img}.pgm ]
then
${CMD_CONVERT} ${DIR_TEST}/$file/${img}.ppm ${img}.pgm # probably use a abs path for output as well?
if [ $? -ne 0 ]
then
echo " ${CMD_CONVERT} failed, test image not converted"
exit -1
else
echo " converted ${file} to PGM"
fi
fi
done
done

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

Successfully merging this pull request may close these issues.

[bug] s_filtergrid.cu does not compile missing thrust/host_vector.h header
3 participants