| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
different interpolation by double image #23124
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
Conversation
|
@vicsyl, thank you for the contribution! The change seems reasonable. I wonder, whether this change in |
| Mat dbl; | ||
| #if DoG_TYPE_SHORT | ||
| resize(gray_fpt, dbl, Size(gray_fpt.cols*2, gray_fpt.rows*2), 0, 0, INTER_LINEAR_EXACT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INTER_LINEAR_EXACT
Main problem here is that we already have declared the "bit-exact" implementation for SIFT.
"Silent" replacing of such implementation with new one is a regression in term of API interface stability.
I believe we should have an option for enabling of fixed version and a warning about the deprecated behavior.
/cc @vpisarev
Somehow I cannot reply to this directly. "Natural scaling" can be achieved via warpAffine - now present in SIFT but it is also used in ASIFT, so I don't see a problem there. https://github.com/vicsyl/dog_precision/blob/master/OpenCV%20homography%20estimation.ipynb |
|
ok, since we now have a flag that switches between the modes and so the original behaviour can still be achieved, I suggest to merge it |
| @@ -37,7 +37,7 @@ INSTANTIATE_TEST_CASE_P(AKAZE_DESCRIPTOR_KAZE, DetectorRotationInvariance, | |||
| */ | |||
|
|
|||
| INSTANTIATE_TEST_CASE_P(SIFT, DetectorScaleInvariance, | |||
| Value(IMAGE_BIKES, SIFT::create(0, 3, 0.09), 0.65f, 0.98f)); | |||
| Value(IMAGE_BIKES, SIFT::create(0, 3, 0.09), 0.60f, 0.98f)); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this regression in accuracy?
|
@vicsyl, the PR has been discussed at today's core meeting. It is almost ready for the merge. 2 things need to be done:
|
done both |
* different interpolation by double image * fixing scaling mapping * fixing a test * added an option to enable previous interpolation * added doxygen entries for the new parameter * ASSERT_TRUE -> ASSERT_EQ * changed log message when using old upscale mode
* different interpolation by double image * fixing scaling mapping * fixing a test * added an option to enable previous interpolation * added doxygen entries for the new parameter * ASSERT_TRUE -> ASSERT_EQ * changed log message when using old upscale mode
Fixing #23123
See also: https://github.com/vicsyl/dog_precision
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
Even though by detector scale invariance test for SIFT the constrains had to be relaxed for the test to pass, the change improved other test metrics in SIFT detector/descriptor/scale/rotation invariance tests. Even metrics for detector scale invariance test for SIFT are better after the change for different parameters. See my comment at test_detectors_invariance.cpp for more details.
Note that the scaling of keypoint locations was changed so that is corresponds to homography:
See my comment in test_invariance_utils.hpp.
The performance tests didn't show performance regression: