minor suggestions for code improvement

[mitjap]

Shouldn't this check bi inside of ModeFunctions<Config::OpenCV>::refine function?

popsift/src/popsift/s_extrema.cu

Line 447 in fafcad9

if (iter >= MAX_ITERATIONS) {

There is already a comment in place suggesting this:

popsift/src/popsift/s_extrema.cu

Line 160 in fafcad9

// Or we quit the loop by exceeding the limit, and reject the point anyway.

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.

[mitjap]

popsift/src/popsift/s_extrema.cu

Line 203 in 74f7053

return ( fabsf( val ) >= 0.8f * 2.0f * d_consts.threshold );

popsift/src/popsift/s_extrema.cu

Line 255 in f63d305

return ( fabsf( val ) >= 1.6f * d_consts.threshold );

popsift/src/popsift/s_extrema.cu

Line 485 in f63d305

if( fabsf(contr) < scalbnf( d_consts.threshold, 1 ) )

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.

popsift/src/popsift/sift_conf.cu

Line 278 in f63d305

return ( _threshold * 0.5f * 255.0f / levels );

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.

[mitjap]

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.

popsift/src/popsift/s_pyramid_build_ra.cu

Line 54 in f63d305

surf2DLayeredwrite( out * 255.0f, dst_data, write_x*4, write_y, 0, cudaBoundaryModeZero );

popsift/src/popsift/s_pyramid_build_ra.cu

Line 90 in f63d305

surf2DLayeredwrite( out * 255.0f, dst_data, write_x*4, write_y, level, cudaBoundaryModeZero );

popsift/src/popsift/sift_conf.cu

Line 278 in f63d305

return ( _threshold * 0.5f * 255.0f / levels );

popsift/src/popsift/s_pyramid_fixed.cu

Line 174 in f63d305

fval *= 255.0f; // don't forget to upscale

popsift/src/popsift/sift_conf.h

Line 343 in 3cd0836

/// default threshold 0.04 / (_levels-3.0) / 2.0f * 255

[mitjap]

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.

popsift/src/popsift/s_extrema.cu

Line 279 in f63d305

n.x += t.x;

popsift/src/popsift/s_extrema.cu

Line 461 in f63d305

const float xn = n.x + d.x;

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;

OpenCVmode should have something like this:

d.x -= roundf( d.x );
d.y -= roundf( d.y );
d.z -= roundf( d.z );

[griwodz]

@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!

[stale]

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.