Open mitjap opened 3 years ago
https://github.com/alicevision/popsift/blob/74f705301257ca00e096f62978505c3d8e078e3e/src/popsift/s_extrema.cu#L203
https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/s_extrema.cu#L255
https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/s_extrema.cu#L485
I also don't think this 2.0f
factor should be here. I understand that it is here just to negate some multiplication 0.5f
which is made in Config::getPeakThreshold
.
https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/sift_conf.cu#L278
Looking at commit history this was made to adapt algorithm to OpenCV
behaviour so it is reasonable to put this multiplication in ModeFunctions<Config::OpenCV>::first_contrast_ok
and remove it elsewhere.
Is there a reason that values values in textures are multiplied by 255.0f
? Float values are usually in range [0-1]. I don't see any reason for doing so and I think can be removed.
https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/s_pyramid_build_ra.cu#L54 https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/s_pyramid_build_ra.cu#L90 https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/sift_conf.cu#L278 https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/s_pyramid_fixed.cu#L174 https://github.com/alicevision/popsift/blob/3cd0836df31cb29ead80cd182910a213e9e2c785/src/popsift/sift_conf.h#L343
I think there might be an error in some cases.
In case where algorithm reaches MAX_ITERATIONS then variable n
might have been modified in ModeFunctions<>::refine
by -1
or +1
if d
is >0.6
. Later on n
is again modified by d
even though part of d
was already taken into account in refine
.
https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/s_extrema.cu#L279
https://github.com/alicevision/popsift/blob/f63d305131971c2cf96b43468304f1bfea25cb30/src/popsift/s_extrema.cu#L461
To fix this issue something like this should be added to refine
function for PopSift
and VLFeat
modes.
d.x -= t.x;
d.y -= t.y;
d.z -= t.z;
OpenCV
mode should have something like this:
d.x -= roundf( d.x );
d.y -= roundf( d.y );
d.z -= roundf( d.z );
@mitjap Sorry that I'm silent - end of semester, examining, grading, admission of new students etc. I'm glad that someone is giving the code a thorough review!
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Shouldn't this check bi inside of
ModeFunctions<Config::OpenCV>::refine
function? https://github.com/alicevision/popsift/blob/fafcad973bd7b1740acd4c276922fe039516a787/src/popsift/s_extrema.cu#L447There is already a comment in place suggesting this: https://github.com/alicevision/popsift/blob/fafcad973bd7b1740acd4c276922fe039516a787/src/popsift/s_extrema.cu#L160
Hope I'm not nitpicking here :) Just trying to make code be more readable. I'm also aware this should probably just be PR but I don't really have time to test and verify this claim.