Skip to content

Improvement on error handling for HoughCircles#22466

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
sturkmen72:patch-4
Sep 6, 2022
Merged

Improvement on error handling for HoughCircles#22466
asmorkalov merged 1 commit intoopencv:4.xfrom
sturkmen72:patch-4

Conversation

@sturkmen72
Copy link
Copy Markdown
Contributor

@sturkmen72 sturkmen72 commented Sep 3, 2022 •
edited
Loading

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • 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
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • 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

@sturkmen72 sturkmen72 marked this pull request as draft September 3, 2022 22:01
asmorkalov
asmorkalov reviewed Sep 5, 2022
View reviewed changes
@sturkmen72
Copy link
Copy Markdown
Contributor Author

sturkmen72 commented Sep 5, 2022

i think if the following changes will be better

            if( param2 >= 1 )
                if( param2 == 100 )
                    param2 = 0.9
                else
                    CV_Error( Error::StsOutOfRange, "param2 parameter must be smaller than 1.0" );

because there is a default value for param2 = 100 ideal for HOUGH_GRADIENT but not for HOUGH_GRADIENT_ALT

for example
circles = cv2.HoughCircles(gray_img, cv2.HOUGH_GRADIENT,1, 120)
woks well

circles = cv2.HoughCircles(gray_img, cv2.HOUGH_GRADIENT_ALT,1, 120)
will raise an exception

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Sep 5, 2022

No, function should not change it's parameters on the go. It's not expected behavior. Proper documentation and run time range checks are the best solution.

@sturkmen72 sturkmen72 marked this pull request as ready for review September 5, 2022 16:01
@asmorkalov asmorkalov self-requested a review September 5, 2022 16:16
@asmorkalov asmorkalov self-assigned this Sep 6, 2022
asmorkalov
asmorkalov approved these changes Sep 6, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@asmorkalov asmorkalov merged commit b26fc6f into opencv:4.x Sep 6, 2022
@sturkmen72 sturkmen72 deleted the patch-4 branch September 6, 2022 09:14
@alalek alalek mentioned this pull request Jan 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@asmorkalov asmorkalov

Labels

category: imgproc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sturkmen72 @asmorkalov