Hough_Line_Probabilistic Added

[vernwalrahul]

Implemented Hough_Line_Probabilistic in hough_transform.jl as proposed in the following paper:
Link.

Following are some of the results:

[zygmuntszpak]

Thank you for taking the time to make a contribution. I've downloaded the paper so I can follow the code better.

[codecov]

Codecov Report

Merging #49 into master will increase coverage by 1.07%.
The diff coverage is 99.23%.

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   97.28%   98.35%   +1.07%     
==========================================
  Files          10       10              
  Lines         405      792     +387     
==========================================
+ Hits          394      779     +385     
- Misses         11       13       +2

Impacted Files	Coverage Δ
src/houghtransform.jl	99.53% <99.23%> (-0.47%)	⬇️
src/freak.jl	98.48% <0%> (-1.52%)	⬇️
src/brief.jl	100% <0%> (ø)	⬆️
src/lbp.jl	100% <0%> (ø)	⬆️
src/corner.jl	100% <0%> (ø)	⬆️
src/brisk.jl	100% <0%> (ø)	⬆️
src/core.jl	100% <0%> (ø)	⬆️
src/glcm.jl	100% <0%> (ø)	⬆️
src/orb.jl	97.36% <0%> (+1.71%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a1be90...5c05ba1. Read the comment docs.

[vernwalrahul]

I have updated the code adding comments and reference. Can you please review this PR?
ping @zygmuntszpak @timholy

[zygmuntszpak]

Could you also add some tests for this function please? Thanks for adding the comments---they will make it easier to verify the implementation. I haven't had the time yet to think about this more deeply, but at first glance your function seems very long and hence difficult to parse visually. Perhaps some of the steps ought to go into separate "helper" functions?

[vernwalrahul]

@zygmuntszpak
Thanks for the comments. I will try adding separate helper functions.
And since the algorithm is dependent on randomly selected points, it may turn up with different line segments each time we run it. I wonder if adding test cases will suffice the purpose.

[zygmuntszpak]

For the most basic validation you could organise your test so that there is only one line segment and maybe a few other scattered random points. You can seed the random number generator with a particular seed so that every time the experiment is run the same set of random points are generated. Presumably the algorithm should always find the only line segment in the example?

You could then try another test with two line segments.

Also, instead of error(...) use throw(ArgumentError(...))

[vernwalrahul]

@zygmuntszpak I have added the required helper functions along with few test cases, as suggested.

[zygmuntszpak]

Thank you Rahul. Could you please move the functions that are currently defined inside hough_line_probabilistic outside of hough_line_probabilistic. For example, move function collect_points() outside of hough_line_probabilistic. The aim is to make hough_line_probabilistic as short as possible so that it can, ideally, fit onto a single page. The brevity will help people who are encountering the code for the first time parse the logic of the algorithm.

[vernwalrahul]

@zygmuntszpak Sure, I can do that. But that will increase the number of arguments, I think. How about adding a separate file as "hough_line_submodules.jl" ? I can import required data and functions from there only. There is one more function in hough_line_standard written in nested form.

[zygmuntszpak]

I'm not sure I fully understand your separate file idea. I don't know to how best to resolve this, but I do feel that the code is one very long piece which gets in the way of interpreting its intent. Perhaps you could group some of the parameters in a Type if you feel the argument list to the helper functions is getting too long?

[vernwalrahul]

@zygmuntszpak I have moved the nested functions outside, passing the arguments after wrapping the various parameters.

[zygmuntszpak]

I've checked your branch out and used the "Traceur.jl" package to check for Type stability. In particular, I ran @trace lines = hough_line_probabilistic(img, 1, linspace(0,π,180),9,7,15,4) using the img associated with the #for a square image part of your test code.

The @trace command found some issues. Here is the output of the analysis:

@trace lines = hough_line_probabilistic(img, 1, linspace(0,π,180),9,7,15,4)
(ImageFeatures.update_accumulator)(::ImageFeatures.Param, ::Tuple{Int64,Int64}, ::Array{Float64,1}, ::Array{Float64,1}) at /home/zygmunt/.julia/v0.6/ImageFeatures/src/houghtransform.jl:188
  dist is assigned as Int64 at line 191
  dist is assigned as Float64 at line 192
  dist is assigned as Int64 at line 193
  val is assigned as Any at line 195
  max_val is assigned as Int64 at line 189
  max_val is assigned as Any at line 197
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :accumulator_matrix), (Base.add_int)(n, 1), (Base.add_int)(_7::Int64, 1)) at line 194
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :accumulator_matrix), (Base.add_int)(n, 1), (Base.add_int)(_7::Int64, 1)) + 1 at line 194
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :accumulator_matrix), (Base.add_int)(n, 1), (Base.add_int)(_7::Int64, 1)) at line 195
  dynamic dispatch to max_val < val at line 196
  returns Tuple{Int64,Any}
(Base.indexed_next)(::Tuple{Int64,Bool}, ::Int64, ::Int64) at tuple.jl:54
  returns Tuple{Union{Bool, Int64},Int64}
(ImageFeatures.pass_1)(::ImageFeatures.Param, ::ImageFeatures.Sample, ::Int64) at /home/zygmunt/.julia/v0.6/ImageFeatures/src/houghtransform.jl:206
  h is assigned as Any at line 207
  w is assigned as Any at line 207
  #temp# is assigned as Any at line 207
  #temp# is assigned as Any at line 207
  #temp# is assigned as Any at line 207
  #temp# is assigned as Bool at line 232
  #temp# is assigned as Any at line 232
  #temp# is assigned as Bool at line 232
  #temp# is assigned as Any at line 232
  #temp# is assigned as Any at line 232
  #temp# is assigned as Any at line 232
  dynamic dispatch to (ImageFeatures.size)((Core.getfield)(params, :mask)) at line 207
  dynamic dispatch to (Base.start)((ImageFeatures.size)((Core.getfield)(params, :mask))) at line 207
  dynamic dispatch to (Base.indexed_next)((ImageFeatures.size)((Core.getfield)(params, :mask)), 1, #temp#) at line 207
  dynamic dispatch to (Base.indexed_next)((ImageFeatures.size)((Core.getfield)(params, :mask)), 2, #temp#) at line 207
  dynamic dispatch to j1 >= w at line 232
  dynamic dispatch to i1 >= h at line 232
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :mask), (Base.add_int)(i1, 1), (Base.add_int)(j1, 1)) at line 238
(ImageFeatures.hough_line_probabilistic)(::Array{Bool,2}, ::Int64, ::StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}, ::Int64, ::Int64, ::Int64, ::Int64) at /home/zygmunt/.julia/v0.6/ImageFeatures/src/houghtransform.jl:304
  max_val is assigned as Any at line 345
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :mask), (Base.getfield)(point, 1), (Base.getfield)(point, 2)) at line 340
  dynamic dispatch to !((ImageFeatures.getindex)((Core.getfield)(params, :mask), (Base.getfield)(point, 1), (Base.getfield)(point, 2))) at line 340
  dynamic dispatch to max_val < (Core.getfield)(params, :threshold) at line 348
4-element Array{NTuple{4,Int64},1}:
 (3, 3, 3, 14)
 (14, 3, 4, 3)
 (14, 14, 4, 14)
 (14, 4, 14, 13)

Note, for example, the lines:

  dist is assigned as Int64 at line 191
  dist is assigned as Float64 at line 192
  dist is assigned as Int64 at line 193

The problem is that dist is being converted to a Float64 on line 192 by virtue of the fact that params.constadd is declared as a Float64.

In a nutshell, you don't want to to be changing the Type of a variable and furthermore, you don't want a variable to be identified as Any. See the discussion on the performance tips in the Julia documentation. I'm not entirely sure what the cause of #temp# type instability is, but perhaps you could sort out the non-temporary variables in the meantime.

Thank you for your valuable time and effort.

[vernwalrahul]

@zygmuntszpak Thanks for the info. I didn't know about this trace command.
I have corrected the code accordingly, but am unable to check for the issues. I am not sure if I have cloned the right package. Is it https://github.com/MikeInnes/Traceur.jl?

This one is showing the following error:

[zygmuntszpak]

Hmm..not sure what is going on there. Did you do Pkg.add("Traceur") and then

using Traceur
# TODO: define img

@trace lines = hough_line_probabilistic(img, 1, linspace(0,π,180),9,7,15,4) 

[vernwalrahul]

@zygmuntszpak Yes, I did exactly the same thing. It's Traceur v0.1.1 here. I don't know why it's showing the same error with hough_transform_standard as well.

[zygmuntszpak]

That's odd, I am using the same version of Traceur and it works for me. The latest output is:

@trace hough_line_probabilistic(img, 1, linspace(0,π,180),9,7,15,4)
(ImageFeatures.update_accumulator)(::ImageFeatures.Param, ::Tuple{Int64,Int64}, ::Array{Float64,1}, ::Array{Float64,1}) at /home/zygmunt/.julia/v0.6/ImageFeatures/src/houghtransform.jl:188
  dist is assigned as Float64 at line 191
  dist is assigned as Float64 at line 192
  dist is assigned as Int64 at line 193
  val is assigned as Any at line 195
  max_val is assigned as Int64 at line 189
  max_val is assigned as Any at line 197
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :accumulator_matrix), (Base.add_int)(n, 1), (Base.add_int)(_7::Int64, 1)) at line 194
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :accumulator_matrix), (Base.add_int)(n, 1), (Base.add_int)(_7::Int64, 1)) + 1 at line 194
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :accumulator_matrix), (Base.add_int)(n, 1), (Base.add_int)(_7::Int64, 1)) at line 195
  dynamic dispatch to max_val < val at line 196
  returns Tuple{Int64,Any}
(Base.indexed_next)(::Tuple{Int64,Bool}, ::Int64, ::Int64) at tuple.jl:54
  returns Tuple{Union{Bool, Int64},Int64}
(ImageFeatures.pass_1)(::Array{Bool,2}, ::ImageFeatures.Param, ::ImageFeatures.Sample, ::Int64) at /home/zygmunt/.julia/v0.6/ImageFeatures/src/houghtransform.jl:206
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :mask), (Base.add_int)(i1, 1), (Base.add_int)(j1, 1)) at line 238
(ImageFeatures.hough_line_probabilistic)(::Array{Bool,2}, ::Int64, ::StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}, ::Int64, ::Int64, ::Int64, ::Int64) at /home/zygmunt/.julia/v0.6/ImageFeatures/src/houghtransform.jl:304
  max_val is assigned as Any at line 345
  dynamic dispatch to (ImageFeatures.getindex)((Core.getfield)(params, :mask), (Base.getfield)(point, 1), (Base.getfield)(point, 2)) at line 340
  dynamic dispatch to !((ImageFeatures.getindex)((Core.getfield)(params, :mask), (Base.getfield)(point, 1), (Base.getfield)(point, 2))) at line 340
  dynamic dispatch to max_val < (Core.getfield)(params, :threshold) at line 348
4-element Array{NTuple{4,Int64},1}:
 (14, 3, 3, 3)
 (3, 4, 3, 14)
 (14, 14, 4, 14)
 (14, 4, 14, 12)

You can also try @code_warntype hough_line_probabilistic(img, 1, linspace(0,π,180),9,7,15,4)

But the output of that in your case is very long. It does pick up that maxval and val is any. We might have to call in the Calvary on this one ;) @kmsquire When you get a chance, could you please review this code? We're not sure how to resolve the type stability issues.