CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
imgproc: optimise local cost computation in IntelligentScissorsMB::buildMap #21959
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
imgproc: optimise local cost computation in IntelligentScissorsMB::buildMap #21959
Conversation
Here are some more notes on the optimisation. The
and the optimisation uses the non-negativity of the terms and the fact that we don't need the exact cost if the cost exceeds the threshold. E.g. if Supporting links w.r.t. non-negativity of the terms:
|
Thanks for approving the CI run for this pull request! Reviewing the "PR:4.x W10 / BuildAndTest (pull_request) Cancelled after 360m — BuildAndTest" outcome, this was
and apparently because (at the time) there was no https://github.com/cpoerschke/opencv_extra fork in existence. Basic git documentation searching found no |
|
||
INSTANTIATE_TEST_CASE_P(/**/, Imgproc_IntelligentScissorsMB, testing::Combine(testing::Values( true, false ), | ||
testing::Values( 1, 100 ))); |
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.
What is the purpose of adding 100 iterations for the accuracy test?
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.
It was a naive way of getting performance test numbers ... I have now in cd6ef45 added a separate performance test instead.
@cpoerschke Thanks a lot for the patch! Couple of questions for the solution:
|
Hi @asmorkalov - thanks for the review!
Your understanding is correct. The special flag (during development) helps compare performance and accuracy via the parameterised tests but yes beyond that there is technically no need for the flag to remain. Sometimes flags can help preserve existing behaviour for APIs for backwards compatibility between versions or so but that is not the case here I think.
Have just added a commit that modifies (so far only) one test to check ca. 5+5% of the contour values, at the beginning and the end of the contour. |
I propose 2 things:
|
#23688 opened for the first thing - .xml files seemed more common than .yaml files and so I went with that for now. opencv/opencv_extra#1065 is the matching PR for the opencv_extra repo. |
a01344e commit added for the second thing. |
I propose to restore perf test. We can use summary.py and other scripts to compare performance numbers for different versions, e.g. gefore and after patch. |
imgproc: add basic IntelligentScissorsMB performance test #23698 Adding basic performance test that can be used before and after the #21959 changes etc. as per @asmorkalov's #21959 (comment) comment. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [X] I agree to contribute to the project under Apache 2 License. - [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [X] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
imgproc: add contour values check to IntelligentScissorsMB tests Preparation for the #21959 changes as per @asmorkalov's #21959 (comment) suggestion. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [X] I agree to contribute to the project under Apache 2 License. - [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [X] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
@cpoerschke Thanks a lot for the contribution! I merged accuracy and performance tests. Could you rebase the PR with optimization on top of current 4.x and squash commits? |
a01344e
to
f597838
Compare
Thanks @asmorkalov for the review inputs! I've rebased and squashed the commits. Also ran the performance test on the commit before and the commit itself - unless I'm misreading the numbers the optimisation actually makes things slower now. Would like to look into that more thoroughly, perhaps I missed something when removing the switch flag? The accuracy tests pass. |
I see optimization effect, but the mentioned function is not bootle neck:
|
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.
👍
imgproc: add contour values check to IntelligentScissorsMB tests Preparation for the opencv#21959 changes as per @asmorkalov's opencv#21959 (comment) suggestion. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [X] I agree to contribute to the project under Apache 2 License. - [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [X] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
imgproc: add basic IntelligentScissorsMB performance test opencv#23698 Adding basic performance test that can be used before and after the opencv#21959 changes etc. as per @asmorkalov's opencv#21959 (comment) comment. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [X] I agree to contribute to the project under Apache 2 License. - [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [X] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
imgproc: add contour values check to IntelligentScissorsMB tests Preparation for the opencv#21959 changes as per @asmorkalov's opencv#21959 (comment) suggestion. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [X] I agree to contribute to the project under Apache 2 License. - [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [X] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
imgproc: add basic IntelligentScissorsMB performance test opencv#23698 Adding basic performance test that can be used before and after the opencv#21959 changes etc. as per @asmorkalov's opencv#21959 (comment) comment. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [X] I agree to contribute to the project under Apache 2 License. - [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [X] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
imgproc: add contour values check to IntelligentScissorsMB tests Preparation for the opencv#21959 changes as per @asmorkalov's opencv#21959 (comment) suggestion. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [X] I agree to contribute to the project under Apache 2 License. - [X] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [X] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
When reading the code I noticed a
TODO(opt)
comment, this pull request is for that.https://github.com/opencv/opencv/blob/4.5.5/modules/imgproc/src/intelligent_scissors.cpp#L628
Notes
imgproc: fix two typos (imput, magnutude)Â #21941 and this pull request change the same non-test files. However, the changes themselves don't overlap i.e. there should be no merge conflicts.
Based on https://github.com/opencv/opencv/wiki/Branches#which-branch-should-i-target-my-pr this pull request here could be for 3.4 instead of 4.x branch. However, it is for 4.x since 3.4 doesn't have the intelligent scissors code.
Test run snippet
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.