DOC ENH + API fix on houghline transform #2089

Merged
merged 2 commits into from Jun 9, 2016

Conversation

Projects
None yet
5 participants
@sciunto
Member

sciunto commented May 17, 2016

Hi,

Long story short, I got some discrepancies from the Hough line transform. For instance, if you look carefully at the gallery, it doesn't superimpose perfectly. Based on that, I started a code analysis.

The hough line peak detection has a pixel resolution. I wrote a code to make it subpixel. Once I finished a first version of it, I figured out something else.

The default theta value in hough_line() specifies not only a range, but also a bin size. That was not clearly stated in the doc. Reducing the bin size helps to improve the accuracy of the detection. Quick code that shows that:

for i in range(1, 11):
    print(i*90)
    h, theta, d = hough_line(image, theta=np.linspace(-np.pi / 2, np.pi / 2, 90*i))
    angles = np.array([angle for _, angle, dist in zip(*hough_line_peaks(h, theta, d))])
    print((np.abs(angles) * 180 / np.pi - 45) / 45. * 100)
90
[ 1.12359551  1.12359551]
180
[-0.55865922 -0.55865922]
270
[ 0.37174721  0.37174721]
360
[-0.27855153 -0.27855153]
450
[-0.66815145  0.22271715]
540
[ 0.55658627 -0.18552876]
630
[-0.47694754  0.15898251]
720
[ 0.41724618 -0.13908206]
810
[ 0.12360939  0.12360939]
900
[-0.55617353 -0.11123471]

Modification 1 So, I modified the docstring to hightlight that point.
It's trivial at the end, but took me a while to figure that out.

Modification 2 I changed the "returned values" in the docstring to be consistent with the notations for the peak detection.

Modification 3 I noticed that the cython function had a None default value. This is not handled by the code (Didn't try, but I think it would crash). The None case is handled by the python wrapper function. So I removed that. It's an API modification, but it's an internal function, I guess we are fine with that.

To come back on the subpixel algo, at the end, it shows a better result than the actual algorithm.
I just did a local peak fit with a gaussian curve. The idea here is to use the information from neighbouring pixels. If you think it's worth it, I can work on a separate PR to add this feature.
I may try with a lorenzian instead. usually, I have good results with lorenzian functions for data that fades out quickly with a good baseline.

I ping @stefanv @emmanuelle for comments, others are welcome to comment too ;)

Returns
-------
- H : 2-D ndarray of uint64
+ hspace : 2-D ndarray of uint64

This comment has been minimized.

@soupault

soupault May 17, 2016

Member

I'm pretty sure the output variables are named this way to be consistant with MATLAB signatures. 👎 from my side.

@soupault

soupault May 17, 2016

Member

I'm pretty sure the output variables are named this way to be consistant with MATLAB signatures. 👎 from my side.

This comment has been minimized.

@sciunto

sciunto May 17, 2016

Member

I'm sorry, but I personally don't care to be consistent to their signatures and not at the price to be inconsistent with ourself :) Why them and not others? I never used matlab and while they are doing non foss softwares, I'm not going to pay attention to what they do. In addition, @stefanv recently said that to build something instead of reproducing (cf recent video posted the mailing list). I think it's a solid point if we want to make progress

@sciunto

sciunto May 17, 2016

Member

I'm sorry, but I personally don't care to be consistent to their signatures and not at the price to be inconsistent with ourself :) Why them and not others? I never used matlab and while they are doing non foss softwares, I'm not going to pay attention to what they do. In addition, @stefanv recently said that to build something instead of reproducing (cf recent video posted the mailing list). I think it's a solid point if we want to make progress

This comment has been minimized.

@JDWarner

JDWarner May 17, 2016

Contributor

Considering the actual variable names here don't matter to the user in any way, I'm fine with being more descriptive and following capitalization conventions. distances might be better than dists though.

@JDWarner

JDWarner May 17, 2016

Contributor

Considering the actual variable names here don't matter to the user in any way, I'm fine with being more descriptive and following capitalization conventions. distances might be better than dists though.

This comment has been minimized.

@ahojnnes

ahojnnes May 18, 2016

Member

It is in theory a backwards-incompatible change though, if people use kwargs.

@ahojnnes

ahojnnes May 18, 2016

Member

It is in theory a backwards-incompatible change though, if people use kwargs.

This comment has been minimized.

@sciunto

sciunto May 18, 2016

Member

@JDWarner I agree with you that distances is better but I hesitated to change the API of the hough peak function. I can change that way depending on how the discussion ends.

@sciunto

sciunto May 18, 2016

Member

@JDWarner I agree with you that distances is better but I hesitated to change the API of the hough peak function. I can change that way depending on how the discussion ends.

This comment has been minimized.

@JDWarner

JDWarner May 18, 2016

Contributor

Isn't the current name distances, while this PR changes it to dists?

@JDWarner

JDWarner May 18, 2016

Contributor

Isn't the current name distances, while this PR changes it to dists?

This comment has been minimized.

@sciunto

sciunto May 18, 2016

Member

@JDWarner We are not on the same frequency :)

I changed the name of hough_line output to match the name used for hough_line_peak input. Ideally, I would have preferred to name everything 'distances' rather than 'dists'. If I do so, I need to change the API of hough_line_peak.

Hope my explanation is better ;)

@sciunto

sciunto May 18, 2016

Member

@JDWarner We are not on the same frequency :)

I changed the name of hough_line output to match the name used for hough_line_peak input. Ideally, I would have preferred to name everything 'distances' rather than 'dists'. If I do so, I need to change the API of hough_line_peak.

Hope my explanation is better ;)

This comment has been minimized.

@soupault

soupault May 20, 2016

Member

Let it be hspace, angles, distances then.
Any objections?

@soupault

soupault May 20, 2016

Member

Let it be hspace, angles, distances then.
Any objections?

This comment has been minimized.

@jni

jni May 22, 2016

Contributor

hspace, angles, distances sounds good to me.

Just to clarify, consistency with Matlab is not a primary design concern for scikit-image's API. =)

@jni

jni May 22, 2016

Contributor

hspace, angles, distances sounds good to me.

Just to clarify, consistency with Matlab is not a primary design concern for scikit-image's API. =)

This comment has been minimized.

@sciunto

sciunto May 22, 2016

Member

OK, I like it too. We are three. I update :)

@sciunto

sciunto May 22, 2016

Member

OK, I like it too. We are three. I update :)

skimage/transform/hough_transform.py
@@ -17,15 +17,15 @@ def hough_line(img, theta=None):
Input image with nonzero values representing edges.
theta : 1D ndarray of double
Angles at which to compute the transform, in radians.
- Defaults to -pi/2 .. pi/2
+ Defaults to -pi/2 .. pi/2 with 180 values.

This comment has been minimized.

@soupault

soupault May 17, 2016

Member

Maybe something like Defaults to a vector of 180 angles evenly spaced from -pi/2 to pi/2 . could be a better wording. What do you think?

@soupault

soupault May 17, 2016

Member

Maybe something like Defaults to a vector of 180 angles evenly spaced from -pi/2 to pi/2 . could be a better wording. What do you think?

This comment has been minimized.

@sciunto

sciunto May 17, 2016

Member

yep!

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto May 18, 2016

Member

I updated the wording.

Member

sciunto commented May 18, 2016

I updated the wording.

François Boulogne added some commits May 17, 2016

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto May 22, 2016

Member

Updated and I rebased to squash the last commits

Member

sciunto commented May 22, 2016

Updated and I rebased to squash the last commits

@soupault

This comment has been minimized.

Show comment
Hide comment
@soupault

soupault May 22, 2016

Member

This LGTM now 👍.

Member

soupault commented May 22, 2016

This LGTM now 👍.

@sciunto sciunto referenced this pull request May 25, 2016

Merged

ENH: generalize hough_peak functions #2109

6 of 6 tasks complete
@ahojnnes

This comment has been minimized.

Show comment
Hide comment
@ahojnnes

ahojnnes Jun 9, 2016

Member

Thanks.

Member

ahojnnes commented Jun 9, 2016

Thanks.

@ahojnnes ahojnnes merged commit 9397f99 into scikit-image:master Jun 9, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sciunto sciunto deleted the sciunto:bugfix branch Jun 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment