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

Added Hough Line Transform #601

Closed

Conversation

annimesh2809
Copy link
Contributor

Added a new feature detection : Hough line transform as mentioned in #600 .

The method returns an array of tuples containing (r,theta) as parametric values of lines present in the image.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Very nice. Needs some tests. I also wonder if this would be better in ImageFeatures.jl or ImageTransforms.jl? ImageFeatures is not yet registered (and not yet updated to the new Images framework), but we should do that.

min_theta < max_theta || error("max_theta must be greater than min_theta")

if ! is_binary_image(img)
img = canny(img)
Copy link
Member

Choose a reason for hiding this comment

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

This breaks orthogonality; what if someone wants a different algorithm than canny? Will be solved if we simply require a Bool eltype.

Hmm, I hadn't noticed that canny returns an Array{Float64}. We should probably change it to return an Array{Bool}.

rho > 0 || error("radius threshold must be positive")
min_theta < max_theta || error("max_theta must be greater than min_theta")

if ! is_binary_image(img)
Copy link
Member

Choose a reason for hiding this comment

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

Better to just require an T<:Union{Bool,Gray{Bool}} eltype.

img = canny(img)
end

width::Int64 = size(img)[2]
Copy link
Member

Choose a reason for hiding this comment

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

No need to declare types like this.


function hough_transform_standard{T}(
img::AbstractArray{T,2},
rho::Float64, theta::Float64,
Copy link
Member

Choose a reason for hiding this comment

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

Too restrictive on the types? Particularly for people who have 32-bit machines, Int64 is not native (better to use Int). Number and Integer might even be best.

width::Int64 = size(img)[2]
height::Int64 = size(img)[1]
irho::Float64 = 1 / rho;
numangle::Int64 = round((max_theta - min_theta)/theta)
Copy link
Member

Choose a reason for hiding this comment

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

round(Int, x) returns an Int. You can use this in several places below, too.

fill!(accumulator_matrix, 0)

#Pre-Computed sines and cosines in tables
tabsin = Array{Float64}(numangle)
Copy link
Member

Choose a reason for hiding this comment

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

Even better, Vector{Float64} (here and below).


#Hough Transform implementation
for pix in CartesianRange(size(img))
if img[pix] == 1.0
Copy link
Member

Choose a reason for hiding this comment

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

If you're using Bool eltypes, this becomes just if img[pix]

if img[pix] == 1.0
for i in 1:numangle
dist::Int64 = round((pix[1] * tabsin[i] + pix[2] * tabcos[i]))
dist += round((numrho -1)/2)
Copy link
Member

Choose a reason for hiding this comment

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

This constant could be hoisted out of the loop.


#Finding local maximum lines
validLines = Array{CartesianIndex}(0)
for val in CartesianRange(size(accumulator_matrix))
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should add a function input to findlocalmaxima and just use that?

#Sorting by value in accumulator_matrix
sort!(validLines, by = (x)->accumulator_matrix[x], rev = true)

linesMax = min(linesMax, size(validLines)[1])
Copy link
Member

Choose a reason for hiding this comment

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

can just use length instead of size(validLines)[1] (they're equivalent if validLines is a vector).

@annimesh2809
Copy link
Contributor Author

Is there any function available for drawing lines using the normal parameters generated by hough transform? I found the package Cairo.jl but is there something simpler than that.

There is an ImageDraw.jl package and a line function implemented in it. Should I write a function to draw line from normal parameters which uses this existing function?

@annimesh2809
Copy link
Contributor Author

@timholy Yes, I also think it would be better to have it in ImageFeatures.jl instead of having it directly in Images. Should I create a new pull request in that repo? Also what updates are required in the ImageFeatures.jl package related to the new images framework, maybe I can help :)

@timholy
Copy link
Member

timholy commented Mar 3, 2017

@annimesh2809, I suspect ImageDraw would indeed be the better way to go...fewer dependencies.

Great, I think ImageFeatures is the best place. With regards to updating ImageFeatures, if you install it and type using ImageFeatures you'll already notice a couple of deprecation warnings that should be fixed. The next step would be to run the tests and see if there are more depwarns or breakages. Probably just getting it updated should be a PR on its own, then new stuff can be added.

@mronian
Copy link
Contributor

mronian commented Mar 3, 2017

@annimesh2809 It would be great if you work on updating ImageFeatures to the new Images.jl. I have been wanting to do that for a long time but haven't been able to get time. I'd be happy to help in case you need any 😄

@annimesh2809
Copy link
Contributor Author

@mronian I would be really happy to update ImageFeatures.jl. But I did not understand what you meant by update ImgaeFeatures to the new Images.jl

@timholy
Copy link
Member

timholy commented Mar 4, 2017

The deprecation warnings and test errors should basically tell you what to do.

@timholy
Copy link
Member

timholy commented Mar 4, 2017

I realized that it may not be obvious that to you "the new Images" is what you're running now. There was a big revamp described in #542 and merged in #577. ImageFeatures was written before that work was even started.

@mronian
Copy link
Contributor

mronian commented Mar 4, 2017

These links might help -

@annimesh2809
Copy link
Contributor Author

I have started working on updating ImageFeatures.jl to work with the new images. But there are some failing test cases without changing any code:

img1
img2

Should I comment out those test cases and create a log of them so that we can correct those test cases later when all the deprecated warnings have been removed?
Also should I create a new issue on ImageFeatures.jl so we can discuss the progress of this update?

@mronian
Copy link
Contributor

mronian commented Mar 4, 2017

I guess some changes to Images.jl have modified some values of the descriptor. This should be solved if take an approximate equals instead of equals. Lets close this thread and move it to ImageFeatures.jl.

@mronian
Copy link
Contributor

mronian commented Mar 4, 2017

@annimesh2809 Lets continue this discussion at JuliaImages/ImageFeatures.jl#24

@mronian
Copy link
Contributor

mronian commented Mar 4, 2017

Whoops. I did not realise this was a PR for hough lines. I thought it was just an issue. I'll reopen it.

@mronian mronian reopened this Mar 4, 2017
@annimesh2809
Copy link
Contributor Author

annimesh2809 commented Mar 4, 2017

@mronian Actually we should close this PR because hough lines will be added to ImageFeatures.jl as discussed above once we make it function without errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants