JuliaImages / ImageTracking.jl

Other
26 stars 8 forks source link

Corrects Int64 to Int in lucas-kanade #4

Closed arijitkar98 closed 6 years ago

arijitkar98 commented 6 years ago

This should allow the build to pass in win32 @zygmuntszpak

codecov[bot] commented 6 years ago

Codecov Report

Merging #4 into master will increase coverage by 1.65%. The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #4      +/-   ##
=========================================
+ Coverage   89.25%   90.9%   +1.65%     
=========================================
  Files           2       2              
  Lines         121     121              
=========================================
+ Hits          108     110       +2     
+ Misses         13      11       -2
Impacted Files Coverage Δ
src/lucas_kanade.jl 90.75% <80%> (+1.68%) :arrow_up:

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 0025214...4f95472. Read the comment docs.

zygmuntszpak commented 6 years ago

The win32 version still has not passed the built tests. Looking at the results there doesn't seem to be a build error. It looks like the build failed because the code failed to complete within the default time limit of an hour. You can check out what I mean by looking at the AppVeyor log file. Is it possible that the code is getting stuck in some kind of infinite loop on win32?

I suggest you install the win32 version of Julia on your local machine and step through the code using the atom step-by-step debugger to verify that it works.

It would also be useful to do some tests with real data. I suggest you use the yosemite sequence which you can download here. In order to import the ground truth (which is in a MATLAB .MAT file) you can use the MAT.jl package. Actually, you might want to load the .MAT file in your own script and then save the groundtruth into a Julia file format with JDL.jl. That way we don't add an unnecessary dependency on .MAT into the project.

arijitkar98 commented 6 years ago

I checked the lucas-kanade function on win32 , it is working fine but is a little slow. What can be done regarding the time limit of the tests?

I have also added the yosemite test sequence for lucas-kanade.

arijitkar98 commented 6 years ago

@zygmuntszpak I don't quite understand the HDF5 installation error in macOS. Need some help to correct this.

zygmuntszpak commented 6 years ago

It looks like others have come across a problem building HDF5 on MAC. As far as I can tell we can't use the proposed workaround in this instance.

In light of the build issues I suggest that you use JDL2.jl instead which does not require the HDF5 C library.

arijitkar98 commented 6 years ago

@zygmuntszpak What do we do regarding the fact that the win32 tests don't complete within the time limit?

zygmuntszpak commented 6 years ago

I'm not sure if there is anything we can do to extend the time on the free version given that the message is: Build execution time has reached the maximum allowed time for your plan (60 minutes).

I am working with the code today to see if I can uncover any obvious performance bottleneck. The code seems to run quite slow even on the 64 bit version, and so I wonder if the timeout on the 32 is indicative of a deeper issue.

Currently, we are getting deprecation warnings for the line G_inv = pinv(G) which is in the middle of a critical loop.

[00:09:04] Stacktrace:
[00:09:04]  [1] depwarn(::String, ::Symbol) at .\deprecated.jl:70
[00:09:04]  [2] one(::Type{ColorTypes.Gray{Float64}}) at C:\Users\appveyor\.julia\v0.6\ColorTypes\src\traits.jl:341
[00:09:04]  [3] pinv(::Array{ColorTypes.Gray{Float64},2}) at .\linalg\dense.jl:863
[00:09:04]  [4] optflow(::Array{ColorTypes.Gray{Float64},2}, ::Array{ColorTypes.Gray{Float64},2}, ::ImageTracking.LK{Int32,Float64,Bool}) at C:\Users\appveyor\.julia\v0.6\ImageTracking\src\lucas_kanade.jl:127
[00:09:04]  [5] optical_flow(::Array{ColorTypes.Gray{Float64},2}, ::Array{ColorTypes.Gray{Float64},2}, ::ImageTracking.LK{Int32,Float64,Bool}) at C:\Users\appveyor\.julia\v0.6\ImageTracking\src\optical_flow.jl:21
[00:09:04]  [6] macro expansion at C:\Users\appveyor\.julia\v0.6\ImageTracking\test\optical_flow.jl:52 [inlined]
[00:09:04]  [7] macro expansion at .\test.jl:860 [inlined]
[00:09:04]  [8] anonymous at .\<missing>:?
[00:09:04]  [9] include_from_node1(::String) at .\loading.jl:576
[00:09:04]  [10] include at .\sysimg.jl:14
[00:09:04]  [11] include_from_node1 at .\loading.jl:576
[00:09:04]  [12] include at .\sysimg.jl:14
[00:09:04]  [13] process_options at .\client.jl:305
[00:09:04]  [14] _start at .\client.jl:371
[00:09:04] while loading C:\Users\appveyor\.julia\v0.6\ImageTracking\test\optical_flow.jl, in expression starting on line 34
[00:09:04] WARNING: one(ColorTypes.Gray{Float64}) will soon switch to returning 1; you might need to switch to `oneunit

That is definitely hurting performance.

I benchmarked the first test example:

using Images, TestImages, StaticArrays, OffsetArrays, FileIO, JLD2
# Testing constants
test_image = "mandrill"
number_test_pts = 500
difference = 0.3
max_error_points_percentage = 10
max_allowed_error = 0.1
max_lost_points_percentage = 40

img1 = Gray{Float64}.(testimage(test_image))
img2 = similar(img1)
for i = 1:size(img1)[1]
    for j = 4:size(img1)[2]
        img2[i,j] = img1[i,j-3]
    end
end

corners = imcorner(img1, method=shi_tomasi)
y, x = findn(corners)
a = map((yi, xi) -> SVector{2}(yi, xi), y, x)

srand(9876)
pts = rand(a, (number_test_pts,))
flow, status, err = optical_flow(img1, img2, LK(pts, [SVector{2}(0.0,0.0)], 11, 4, false, 20))

Using @time after running the above code snippet yields

@time optical_flow(img1, img2, LK(pts, [SVector{2}(0.0,0.0)], 11, 4, false, 20))
 63.253176 seconds (39.69 M allocations: 5.585 GiB, 1.72% gc time)

That's a long time and a huge number of allocations for such a small image.

I'll keep digging...

zygmuntszpak commented 6 years ago

I've managed to dramatically reduce the running time and the number allocations of the aforementioned benchmark to

 1.319577 seconds (339.29 k allocations: 253.056 MiB, 2.29% gc time)

You were inadvertently allocating arrays inside your main loops which resulted in a considerable performance hit. Sidestepping the deprecation warning also helped to tremendously reduce the running time.

It looks like it is a bad idea to use the splatting operator ... inside your loops. For example, sum(Ixx[grid_1[1],grid_1[2]]) is better than sum(Ixx[grid_1...]) since the latter allocates additional memory as part of the splatting.

I've also implemented a custom pinv and svd for 2-by-2 StaticArrays so that we don't have to allocate memory when calling the current pinv. I should probably create a pull-request for the new pinv and svd to the StaticArrays package, but because GSOC time constraints I reckon lets just add it to the ImageTracking repository for now.

I've paste the revised lukas_kanade.jl on our Slack channel so that you can incorporate the changes and create a new pull-request. With luck the win32 version will now terminate within the allocated time on Appveyor---we'll see.

arijitkar98 commented 6 years ago

Woah! This is great. I didn't think these many things were hampering the performance. I will put this into a new PR right away.

Got to learn quite a few pitfalls. Will keep these in mind for the later algos.

zygmuntszpak commented 6 years ago

I'm closing this pull-request on account of #7.