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

Improve docstring of Hough transform function #5165

Open
emmanuelle opened this issue Dec 30, 2020 · 6 comments
Open

Improve docstring of Hough transform function #5165

emmanuelle opened this issue Dec 30, 2020 · 6 comments
Labels
📄 type: Documentation Updates, fixes and additions to documentation

Comments

@emmanuelle
Copy link
Member

The docstring of the hough transform functions does not mention that the Hough transform should be applied to a gradient / contour image, so that one often has to perform first a Canny filter or a gaussian gradient magnitude operation. The doc example https://scikit-image.org/docs/dev/auto_examples/edges/plot_circular_elliptical_hough_transform.html?highlight=hough%20transform could also comment on this preprocessing operation (which is performed in the example with a Canny filter).

This issue was inspired by a question on the user forum https://forum.image.sc/t/elliptical-hough-transform-doesnt-work-on-own-images/47052

@grlee77 grlee77 added the 📄 type: Documentation Updates, fixes and additions to documentation label Jan 1, 2021
@alexdesiqueira
Copy link
Member

Thank you for checking that issue, @emmanuelle.
I think this could go on a Notes section in each function. How does that sound?

@grlee77
Copy link
Contributor

grlee77 commented Jan 4, 2021

I think this could go on a Notes section in each function. How does that sound?

It sounds good, but I think the Image parameter itself, should be updated to make it clearer right up front for those who may not read all the way through to the Notes section. I.e. change the current

Input image with nonzero values representing edges.

to something like

Binary input image with nonzero values representing edges (skimage.feature.canny can be used to produce this type of binary edge map).).

@alexdesiqueira
Copy link
Member

skimage.feature.canny can be used to produce this type of binary edge map.

I'd think it's nicer to maybe having something like that on the Notes as well, Greg. I think it'd clutter the docs too much there. I agree this would be nice to have:

Binary input image with nonzero values representing edges.

But I won't oppose if y'all prefer it like Greg suggested 🙂

@alessandro-giusti
Copy link

Also note that the Notes section of the hough_line function, two sentences are quite unclear. In paricular:

  • X and Y axis are horizontal and vertical edges respectively.
  • The distance is the minimal algebraic distance from the origin to the detected line.

In particular, the latter should make it clear that the distance might be negative.

X and Y axis are horizontal and vertical edges respectively.

@mkcor
Copy link
Member

mkcor commented Jan 22, 2021

I don't have a clear preference between @alexdesiqueira's and @grlee77's suggestions. I think it's even clearer to describe the image parameter as:

Binary input image where nonzero values represent edges.

@alexdesiqueira
Copy link
Member

Thanks for that @alessandro-giusti, we'll check it out as well!
Works for me as well, @mkcor. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

No branches or pull requests

5 participants