Skip to content
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

Update Hough ellipse #4999

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

djmcgregor
Copy link

Description

This PR addresses 4 issues. They follow the commits accordingly.

  1. The accumulator currently uses b^2; however, this means that bin sizes are nonuniform. To correct, I suggest we us b instead of b^2. This will also result in a more consistent accuracy and ellipse fit uncertainty.
  2. We can reduce the number of bins used by setting the lower bin limit to the min accumulator value, instead of 0. Also it is simple enough to return the median bin value, instead of the lower bound.
  3. The logic for the major radius a, and radial distance to a third point d, are incorrect. min_size represents a radius, so it is directly comparable to a. Perhaps the docstring should also be updated to clearly state this is a radius. As for d, we should only consider third points which have a radial distance smaller than the presently considered major axis a.
  4. The current orientation logic is unnecessarily complicated, and occasionally swaps a and b when it shouldn't. With updated login in 3, it is simpler to always keep a the major axis, b the minor axis, and adjust the orientation after to maintain it within the [-pi/2 : pi/2] interval.

Checklist

These updates fail 3 tests.

  • test_hough_ellipse_zero_angle I am unsure of the cause of this failure. When I run locally, the function returns the desired y0 position of 15. Error says it returns 17.
  • test_hough_ellipse_non_zero_posangle1 This fails because I actively choose to return orientations in the [-pi/2 : pi/2] range, instead of [-pi : pi]. I think this test should either be updated, or I can adjust the code accordingly.
  • test_hough_ellipse_non_zero_posangle2 This test should be updated. It actively seeks to compare the major axis field to the smaller axis value. The two generated radii are 6 and 12. The test looks for the major axis to be 6. My changes correctly return 12.

Examples of other tests I ran locally that exemplify the failure of the current implementation (and necessitate these changes) include ellipses drawn on a np.zeros([100, 100]) canvas. I used detection parameters min_size=10 and max_size=45

  • ellipse_perimeter(50, 50, 33, 30, -0.28)
  • ellipse_perimeter(45, 51, 35, 30, 1.42)
  • ellipse_perimeter(50, 50, 30, 20, 10/np.pi*180)
  • ellipse_perimeter(50, 50, 30, 20, 45/np.pi*180)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@alexdesiqueira
Copy link
Member

Thank you for your help @djmcgregor, we appreciate it! Some of these changes were made by @cgohlke.
I'll ask for his, or @sciunto's help here. Thanks again!

@sciunto
Copy link
Member

sciunto commented Oct 7, 2020

My overall comment on this PR is positive. These changes look good. However, it significantly changes the output and so far, I think that a deprecation cycle would be necessary, but you can argue against.

Also, any improvement in docstrings is very welcome! :)

@djmcgregor
Copy link
Author

I agree that these changes could significantly alter the results that some users get, especially with the use of b instead of b^2 for the accumulator. I also don't think the concerns addressed in this PR need immediate addressing, especially for general use cases. Personally, I use this for precision measurement, and quickly noticed these issues. If nothing else, this PR can at least serve as a point of reference for others that have similar concerns.

Base automatically changed from master to main February 18, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants