Adding a VLFeat-compliant feature descriptor #167
Conversation
griwodz
added
type:feature
prio:major
in progress
cuda
griwodz
force-pushed
the
dev/vlfeat-like-descriptor
branch
2 times, most recently
from
be5729d to
da9cc32
Compare
griwodz
force-pushed
the
dev/vlfeat-like-descriptor
branch
from
da9cc32 to
08939e0
Compare
|
@simogasp Could we please revisit this one? The develop-branch descriptor extractors work quite well if PopSift is used for everything, but this PR is about creating SIFT descriptors with better vlFeat-compatibility. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Ok I didn't see you already removed nvtx here. Maybe we can merge the nvtx PR #162 , worst case scenario we can rebase and remove the commit abef1d4 from this one and then if you think that the PR is good enough etc we can merge this one. What do you think? |
|
@simogasp I took out the test scripts and proposed making test scripts that actually work as a project for bachelor students. ;-) I built this on top #162 because at that time, it was impossible to compile the same code on Tegra (ARM with integrated GPU) and Intel/Windows. Reintroducing NVTX3 in #168 should not be a challenge, there are no NVTX calls inside the different descriptor alternatives. |
simogasp
force-pushed
the
dev/vlfeat-like-descriptor
branch
from
a188916 to
157fe70
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new VLFeat-compliant feature descriptor to PopSift, implementing descriptor extraction that follows VLFeat's interpretation of the SIFT algorithm rather than PopSift's original implementation based on Lowe's paper. The change aims to improve compatibility with AliceVision, which uses VLFeat descriptors by default.
Key changes:
- Adds a new
VLFeat_Descdescriptor mode that distributes gradient weights across up to 8 histogram bins - Removes NVTX profiling code and dependencies throughout the codebase
- Refactors command-line help text for descriptor modes into a centralized function
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/popsift/s_desc_vlfeat.cu |
New VLFeat-compliant descriptor implementation with GPU kernel |
src/popsift/s_desc_vlfeat.h |
Header file declaring VLFeat descriptor functions |
src/popsift/sift_conf.h |
Adds VLFeat_Desc enum and usage documentation function |
src/popsift/sift_conf.cu |
Implements VLFeat mode parsing and centralized usage text |
src/popsift/sift_desc.cu |
Integrates VLFeat descriptor into main processing pipeline |
src/popsift/s_gradiant.h |
Adds optimized gradient computation for 32-thread blocks |
| Multiple files | Removes NVTX profiling code and dependencies |
| Application files | Updates to use centralized descriptor mode help text |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| get_gradiant32( mod, th, base_x, pix_y, layer_tex, level ); | ||
|
|
||
| mod /= 2.0f; // Our mod is double that of vlfeat. Huh. |
There was a problem hiding this comment.
The comment 'Huh.' is unclear and unprofessional. Consider explaining why the gradient magnitude needs to be halved or reference the specific difference between PopSift and VLFeat implementations.
Copilot uses AI. Check for mistakes.
| else if( text == "vlfeat" ) | ||
| setDescMode( Config::VLFeat_Desc ); | ||
| else | ||
| POP_FATAL( "specified descriptor extraction mode must be one of loop, grid or igrid" ); |
There was a problem hiding this comment.
The error message is outdated and doesn't include all available modes. It should list all valid options: loop, iloop, grid, igrid, notile, vlfeat.
Copilot uses AI. Check for mistakes.
simogasp
changed the title
|
Good if you can propose the project to a student! It would be good to make it in python so it could be easily run on all platforms. I merged the #162, this is rebased wrt the new develop. There are some minor remarks by Copilot about the error messages; I don't know if they are correct. If you want to fix them, then I think we are good to merge. |
typo correction accepted Co-authored-by: Copilot <[email protected]>
griwodz
force-pushed
the
dev/vlfeat-like-descriptor
branch
from
f98bc4a to
bf754cf
Compare
|
@griwodz are we good to merge? |
|
Yes, this is definitely ready for merging. Rebased to develop and the last check showed consistent results. |
|
Oh, I must still update the changelog. |
Agreed. Unfortunately, I didn't get anyone this semester, either. Testing with CMake is apparently not exciting enough. Separate test code written in Python may be easier to sell. |
2 participants
Description
All of PopSift's earlier descriptors were created by reading the Lowe paper again and again, attempting to understand how the descriptors should be implemented. We still believe that we followed the letter of the text, but apparently not the spirit.
Both Changchang Wu's implementation and VLFeat compute the descriptors differently. Furthermore, VLFeat's descriptors are rotated by a quarter circle compared to Changchang Wu's. This descriptor follows the VLFeat interpretation because it is the one that has been used by default in AliceVision.
Features list
Implementation remarks
At this point, the default PopSift behaviour is still to use our old interpretation. It is faster but obviously not exactly in the spirit off Lowe's idea. If this PR is accepted, should the default be vlfeat for compatibility reasons?