minor suggestions for code improvement

[mitjap]

Shouldn't this check bi inside of ModeFunctions<Config::OpenCV>::refine function? https://github.com/alicevision/popsift/blob/fafcad973bd7b1740acd4c276922fe039516a787/src/popsift/s_extrema.cu#L447

There 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.

[mitjap]

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.

[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.

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

[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. 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;

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[bot]]

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.