Hough_Line_Probabilistic Added #49

vernwalrahul · 2018-03-05T12:45:22Z

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

Following are some of the results:

zygmuntszpak · 2018-03-07T05:13:00Z

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

codecov · 2018-03-11T05:22:52Z

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 · 2018-03-11T05:27:06Z

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

zygmuntszpak · 2018-03-11T10:36:10Z

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 · 2018-03-11T11:50:13Z

@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 · 2018-03-11T12:55:15Z

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 · 2018-03-20T11:37:50Z

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

zygmuntszpak · 2018-03-20T12:47:19Z

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 · 2018-03-20T12:54:56Z

@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 · 2018-03-20T13:13:03Z

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 · 2018-03-20T14:40:13Z

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

zygmuntszpak · 2018-03-21T10:52:37Z

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 · 2018-03-22T14:52:11Z

@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:/MikeInnes/Traceur.jl?

This one is showing the following error:

zygmuntszpak · 2018-03-23T12:07:17Z

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 · 2018-03-24T09:12:33Z

@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 · 2018-03-24T10:07:57Z

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.

Hough_Line_Probabilistic Added

c02b225

vernwalrahul force-pushed the hough_prob branch from 6e3580b to 6638323 Compare March 11, 2018 05:30

added comments and reference

6638323

vernwalrahul force-pushed the hough_prob branch from de2ebab to e3b1a08 Compare March 20, 2018 11:17

helper function and test cases added

e3b1a08

update helper functions, wrap parameters

368b91e

removed type instability

5c05ba1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hough_Line_Probabilistic Added #49

Hough_Line_Probabilistic Added #49

vernwalrahul commented Mar 5, 2018 •

edited

Loading

zygmuntszpak commented Mar 7, 2018

codecov bot commented Mar 11, 2018 •

edited

Loading

vernwalrahul commented Mar 11, 2018

zygmuntszpak commented Mar 11, 2018

vernwalrahul commented Mar 11, 2018

zygmuntszpak commented Mar 11, 2018

vernwalrahul commented Mar 20, 2018 •

edited

Loading

zygmuntszpak commented Mar 20, 2018

vernwalrahul commented Mar 20, 2018

zygmuntszpak commented Mar 20, 2018

vernwalrahul commented Mar 20, 2018

zygmuntszpak commented Mar 21, 2018

vernwalrahul commented Mar 22, 2018 •

edited

Loading

zygmuntszpak commented Mar 23, 2018

vernwalrahul commented Mar 24, 2018

zygmuntszpak commented Mar 24, 2018

Hough_Line_Probabilistic Added #49

Are you sure you want to change the base?

Hough_Line_Probabilistic Added #49

Conversation

vernwalrahul commented Mar 5, 2018 • edited Loading

zygmuntszpak commented Mar 7, 2018

codecov bot commented Mar 11, 2018 • edited Loading

Codecov Report

vernwalrahul commented Mar 11, 2018

zygmuntszpak commented Mar 11, 2018

vernwalrahul commented Mar 11, 2018

zygmuntszpak commented Mar 11, 2018

vernwalrahul commented Mar 20, 2018 • edited Loading

zygmuntszpak commented Mar 20, 2018

vernwalrahul commented Mar 20, 2018

zygmuntszpak commented Mar 20, 2018

vernwalrahul commented Mar 20, 2018

zygmuntszpak commented Mar 21, 2018

vernwalrahul commented Mar 22, 2018 • edited Loading

zygmuntszpak commented Mar 23, 2018

vernwalrahul commented Mar 24, 2018

zygmuntszpak commented Mar 24, 2018

vernwalrahul commented Mar 5, 2018 •

edited

Loading

codecov bot commented Mar 11, 2018 •

edited

Loading

vernwalrahul commented Mar 20, 2018 •

edited

Loading

vernwalrahul commented Mar 22, 2018 •

edited

Loading