[WIP]Add boosting tracker #20

Deepank308 · 2019-04-04T10:00:21Z

After going through the current PR for the boosting tracker I have planned to maintain the following structure for tracker implementation :
Let's suppose I'm adding tracker algo algo-name.
Then :

add tracker init and update APIs in tracker.jl.
create a folder /algo-name_tracker/ in the src sub-folder for main algo implementation.
add any basic mathematical computation in the core.jl.
add feature extraction necessary for a tracker in tracker_features.jl.
add tracker model in tracker_model.jl.
add tracker roi sampler in tracker_sampler.jl.
add tracker state estimator implementation in tracker_state_estimator.jl.

I will make incremental changes and include tests where ever required.

@zygmuntszpak please share your views for any change or should I go forward with this?

src/tracker.jl

johnnychen94 · 2019-04-04T10:22:37Z

I think implementing codes in one file or multiple files doesn't make much difference in the beginning since it's in one module, you could just start with two files: tracker.jl(or trackers/tracker.jl) and trackers/algo-name.jl, then do code reorganization when there's a necessity (in this PR or future PR).

src/tracker.jl

codecov · 2019-04-04T10:58:16Z

Codecov Report

Merging #20 into master will decrease coverage by 1.51%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
- Coverage    64.8%   63.28%   -1.52%     
==========================================
  Files           7        8       +1     
  Lines         375      384       +9     
==========================================
  Hits          243      243              
- Misses        132      141       +9

Impacted Files	Coverage Δ
src/tracker.jl	`0% <0%> (ø)`
src/core.jl	`50% <0%> (-2.39%)`	⬇️

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 a83b91f...1f23e78. Read the comment docs.

codecov · 2019-04-04T10:58:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7e329ec). Click here to learn what that means.
The diff coverage is 37.5%.

@@            Coverage Diff            @@
##             master      #20   +/-   ##
=========================================
  Coverage          ?   58.05%           
=========================================
  Files             ?       10           
  Lines             ?      515           
  Branches          ?        0           
=========================================
  Hits              ?      299           
  Misses            ?      216           
  Partials          ?        0

Impacted Files	Coverage Δ
src/ImageTracking.jl	`100% <ø> (ø)`
src/core.jl	`50% <0%> (ø)`
src/tracker/boosting_tracker.jl	`0% <0%> (ø)`
src/tracker/tracker.jl	`0% <0%> (ø)`
src/tracker/tracker_sampler.jl	`57.44% <57.44%> (ø)`

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 7e329ec...fcddb94. Read the comment docs.

src/tracker.jl

johnnychen94 · 2019-04-04T13:18:21Z

@Deepank308 Since you're implementing from scratch, I'd like to get feedback from you on JuliaImages/ImageBinarization.jl#26

Deepank308 · 2019-04-10T15:03:19Z

After going through all the above discusisons and referenced issues, I am thinking of something like this :

abstract type Tracker end
struct TrackerAlgoName <: Tracker end 

function (tracker::TrackerAlgoName)(img, bounding_box)
# initialization code goes here
end

function update_tracker(tracker::TrackerAlgoName, img)
# updation code goes here
end

or,

abstract type Tracker end
struct TrackerAlgoName <: Tracker end 

abstract type RegionOfInterest end
struct BoxROI{T, W<:Integer} <: RegionOfInterest
    img::Array{T,2}
    bound::MVector{4, W}
end

function (tracker::TrackerAlgoName)(roi::BoxROI)
# initialization code goes here
end

function update_tracker(tracker::TrackerAlgoName, img)
# updation code goes here
end

I would then declare the tracker structs in tracker.jl file and put the constructors and update_tracker in the trackers/algo-name_tracker.jl.
Remaining everything remains same as stated in the first comment above.
Please suggest if something else is more desirable.
@zygmuntszpak @arijitkar98

johnnychen94 · 2019-04-10T15:17:41Z

LGTM, this’s just how we organize the way we’re thinking, at this point you could start the coding, and discuss more detailed issues later

johnnychen94 · 2019-04-10T21:27:59Z

In case you misunderstood, the following two pieces of codes are totally different:

https://docs.julialang.org/en/v1/manual/constructors/

function TracerAlgoName(img, bounding_box)
# constructor
end

https://docs.julialang.org/en/v1/manual/methods/#Function-like-objects-1

function (tracker::TracerAlgoName)(img)
# function like object
end

My recommendation is:

use constructor TracerAlgoName(img, bounding_box) instead of init_tracker
implement your algorithm in (tracker::TracerAlgoName)(img) instead of update_tracker(tracker::TracerAlgoName, img) -- this also makes documenting easier
support update_tracker!(tracker::Tracker, img) to make the meaning clearer

Looks like we're good on the first one. But I guess you need something like

mutable struct TrackerAlgoName <: Tracker
    roi::BoxROI
end

or

mutable struct TrackerAlgoName <: Tracker
    img
    bound::BoxROI # or ::MVector
end

you make the choice according to your implementation.

The idea of the second item here is: intuitively, when you input some image to a Tracker, you track this image. So actually we don't need a new function update_tracker.

As for the third item. When we really need an explicit update function to make the meaning clearer, then following the coding style, we should use update!(tracker::TrackerAlgoName, img) or update_tracker!(tracker::TrackerAlgoName,img), and this can be done by a simple one-liner update!(tracker::Tracker, img) = tracker(img)

I'm not sure which name is better: update! or update_trakcer! https://github.com/JuliaImages/Images.jl/issues/767#issuecomment-481877878

If you have some other ideas/thoughts when implementing your algorithm, please don't hesitate to share it with us.

Deepank308 · 2019-04-11T14:20:05Z

@johnnychen94 I have much to do in the initialization part of the tracker. For example - I have to prepare negative and positive samples from image, calculate harr features, initialize the model and its estimation. I read somewhere(one reference is here) that doing heavy computation in constructors is a bad practice to be avoided. But, I don't know much about this as far as Julia is concerned. Experiences welcomed.

So, what I am thinking as of now is to just check the constraints in the constructor while still sticking with a init and update! functions.
Views and suggestions needed.

johnnychen94 · 2019-04-11T14:35:31Z

doing heavy computation in constructors is a bad practice to be avoided

I think you're right here, to save resources, it's usually a good practice to compute or allocate memories only when you really need, e.g., lazy-copy and separated init function here.

johnnychen94 · 2019-04-11T14:54:00Z

I can't wait to see your implementations! 🚀 🚀 🚀

zygmuntszpak · 2019-04-12T05:47:31Z

I wonder whether we shouldn't start incorporating the term Abstract when we define our abstract types.

abstract type AbstractTracker end
struct TrackerAlgoName <: AbstractTracker end 

abstract type AbstractROI end
struct BoxROI{T, W<:Integer} <: AbstractROI
    img::Array{T,2}
    bound::SVector{4, W}
end

I also think it may be more performant to have bound::SVector rather than an MVector. That is, when it comes to updating bound it may be faster to assign it to a new SVector, rather than to mutate it directly. Actually, maybe we could do something like

abstract type AbstractTracker end
struct TrackerAlgoName <: AbstractTracker end 

abstract type AbstractROI end
struct BoxROI{T, ConcreteStaticArray <: StaticArray} <: AbstractROI
    img::Array{T,2}
    bound::ConcreteStaticArray
end

so that we can support MVector and SVector depending on how the BoxROI is constructed.

johnnychen94 · 2019-04-12T06:04:46Z

As for

struct BoxROI{T, ConcreteStaticArray <: StaticArray} <: AbstractROI
    img::Array{T,2}
    bound::ConcreteStaticArray
end

I prefer to define a more general version,

struct BoxROI{T <: AbstractArray, S <: AbstractArray} <: AbstractROI
    img::T
    bound::S
end

and move the usage of StaticArray to implementation details.

src/tracker.jl

src/core.jl

Deepank308 · 2019-04-17T11:35:50Z

I have committed the final API for everyone to have a look. As @johnnychen94 pointed out here it will be good to have bounding_box to be AbstractArray for user convenience and move usage of static arrays into the implementation details. But, I am not able to convert Array{Int, 1} to SVector{4, Int}, not even explicitly, I think struct is preventing me to do so. So skipping SVector right now.

The steps involved in the boosting tracker initialization are :

sampling of positive and negative ROIs.
compute harr features
boosting model
model estimation and updation.

I'll be following these steps for implementation.

Deepank308 · 2019-04-18T20:06:46Z

I will add tests for sampling in the next commit

johnnychen94

Still, I didn't go to the algorithm details since I'm not familiar with that, but I trust you and the workflow of your algorithm looks very descriptive even to me who knows almost nothing about image tracking. 👍

The algorithm details should be reviewed by others.

src/core.jl

src/tracker/boosting_tracker.jl

src/ImageTracking.jl

src/tracker/tracker_sampler.jl

src/tracker/tracker.jl

arijitkar98 · 2019-04-19T05:17:05Z

src/core.jl

+
+abstract type AbstractTrackerTargetState end
+
+mutable struct BoostingTrackerTargetState <: AbstractTrackerTargetState


This should be kept in a separate file as well.

src/ImageTracking.jl

src/tracker/tracker_sampler.jl

src/tracker/tracker.jl

src/tracker/boosting_tracker.jl

src/tracker/tracker_sampler.jl

johnnychen94 · 2019-04-19T07:29:50Z

Things are becoming better now, I'm looking forward to review from @zygmuntszpak

johnnychen94

I left some comments on style and test :)

src/tracker/tracker_sampler.jl

johnnychen94 · 2019-04-20T01:48:50Z

test/tracker/sample_roi.jl

+
+@testset "ROI sampling" begin
+    @info "Running ROI sampling tests"
+    box = BoxROI(ones(300, 300), [125, 125, 175, 175])


To make it less error-prone, I think you need to cover the following types forbox.img:

img = ones(300, 300) testcolors = (RGB,Gray) testtypes = (Float32, Float64, N0f8, N0f16) for C in testcolors, T in testtypes box = BoxROI(img .|> C{T}, [125, 125, 175, 175]) ... end

If the function don't work for N0f8 type, manually convert it to float type in your src codes and document it (or throw an error as early as you can), otherwise, they're annoying here and there from my personal experiences.

zygmuntszpak · 2019-04-20T02:19:48Z

I have some suggestions which I intend to type up later today.

src/tracker/tracker.jl

johnnychen94 · 2019-04-20T06:58:04Z

src/tracker/tracker_sampler.jl

@@ -0,0 +1,100 @@
+abstract type AbstractROI end
+mutable struct BoxROI{T <: AbstractArray, S <: AbstractArray} <: AbstractROI


mutable seems to be an overkill, and in julia we use Tuple for bound type, i.e., NTuple{4,Integer}.

I don't think we should be storing the entire image as a field in BoxROI. An abstract region of interest could just be a set of AbstractRange types, one for each dimensions. It would also not need to be mutable. You just instantiate a new when the ROI changes. I imagine something like this:

struct BoxROI{R₁ <: AbstractRange, R₂} <: AbstractROI span₁::R₁ span₂::R₂ end

Is img used through the entire tracking life? If the answer is yes, we then have some reason to hold it in some struct( e.g., Tracker or BoxROI). If it's only used for initialization purpose, it's better not to do so.

zygmuntszpak

I've made some initial comments but I think there is still a lot more to think about.

It would be very helpful if you could describe all of the key steps that you think the algorithm needs to perform in a comment and how you are currently modelling the problem. I'm not convinced that we need all of these mutable structs, and I'm not sure whether we have the type hierarchy and containment completely correct yet.

We need to clarify what the "hot loops" are going to be, what gets constructed for each frame, and what new work needs to be done for every next frame.

We need to be very careful how we design this if we want to lay the foundations for a performant implementation.

zygmuntszpak · 2019-04-20T12:13:20Z

src/core.jl

+    is_target::Bool
+
+    #Uninitialized
+    responses::Array{T, 2} where T


What is the purpose of responses? If it is needed, then you should declare T as part of the struct, for example:
mutable struct BoostingTrackerTargetState{T <: AbstractArray} <: AbstractTrackerTargetState
responses::T
See: https://docs.julialang.org/en/v1/manual/performance-tips/index.html#Avoid-containers-with-abstract-type-parameters-1

zygmuntszpak · 2019-04-20T12:25:43Z

src/tracker/tracker_sampler.jl

@@ -0,0 +1,100 @@
+abstract type AbstractROI end
+mutable struct BoxROI{T <: AbstractArray, S <: AbstractArray} <: AbstractROI


I don't think we should be storing the entire image as a field in BoxROI. An abstract region of interest could just be a set of AbstractRange types, one for each dimensions. It would also not need to be mutable. You just instantiate a new when the ROI changes. I imagine something like this:

struct BoxROI{R₁ <: AbstractRange, R₂} <: AbstractROI span₁::R₁ span₂::R₂ end

zygmuntszpak · 2019-04-20T12:28:38Z

src/tracker/tracker_sampler.jl

+
+CurrentSampler(overlap::F = 0.99, search_factor::F = 1.8) where{F <: AbstractFloat} = CurrentSampler{F}(overlap, search_factor)
+
+function sample_roi(sampler::CurrentSampler, box::BoxROI)


You would then pass the image as a parameter (instead of retrieving it from BoxROI).

zygmuntszpak · 2019-04-20T12:31:30Z

src/tracker/tracker_sampler.jl

+    sampler.tracked_patch = box.bound
+    sampler.valid_region = SVector{4, Integer}([1, 1, size(box.img, 1), size(box.img, 2)])
+
+    height = box.bound[3] - box.bound[1] + 1


You can write a custom size(b::BoxROI) which returns the width and height as a tuple. Since the BoxROI consists of AbstractRange objects, this will be trivial.

zygmuntszpak · 2019-04-20T12:35:37Z

src/tracker/tracker_sampler.jl

+    sampling_region_max_y = sampling_region_min_y + sampling_region_height - 1
+    sampling_region_max_x = sampling_region_min_x + sampling_region_width - 1
+
+    sampling_region = SVector{4, Integer}([sampling_region_min_y, sampling_region_min_x, sampling_region_max_y, sampling_region_max_x])


This is not a good idea. You are allocating an array by invoking the square brackets [..], only to immediately create another array on the stack. You don't need the square brackets here. Perhaps this ought to just be a range objects anyway? i.e. sampling_region_min_y:sampling_region_max_y and sampling_region_min_x:sampling_region_max_x.

zygmuntszpak · 2019-04-20T12:36:42Z

src/tracker/tracker_sampler.jl

+
+function sample_roi(sampler::CurrentSampler, box::BoxROI)
+    sampler.tracked_patch = box.bound
+    sampler.valid_region = SVector{4, Integer}([1, 1, size(box.img, 1), size(box.img, 2)])


This could just be a tuple taken from AbstractRange type family.

zygmuntszpak · 2019-04-20T12:41:10Z

src/tracker/tracker_sampler.jl

+    height = box.bound[3] - box.bound[1] + 1
+    width = box.bound[4] - box.bound[2] + 1
+
+    sampling_region_min_y = max(0, floor(Integer, box.bound[1] - height*((sampler.search_factor - 1) / 2) + 1))


Instead of all of the .fieldname usage, I think its better to write a get_search_factor(s::Sampler) method which returns the search_factor and assign it once at the top.

search_factor = get_search_factor(sampler) sampling_region_min_y = ...

So that you don't keep writing sampler.search_factor.
In general, try to minimize the number of .field expression and in particular .filed[] expressions. In my view it decreases the readability. The code is unnecessarily lengthy otherwise.

Ok, I'll take care of this.

zygmuntszpak · 2019-04-20T12:43:17Z

src/tracker/tracker_sampler.jl

+    sampling_region_min_x = max(0, floor(Integer, box.bound[2] - width*((sampler.search_factor - 1) / 2) + 1))
+
+    sampling_region_height = min(floor(Integer, height*sampler.search_factor), size(box.img, 1) - sampling_region_min_y + 1)
+    sampling_region_width = min(floor(Integer, width*sampler.search_factor), size(box.img, 2) - sampling_region_min_x + 1)


Please clarify the role of the search_factor. Do we need all of these floor and min operations inside a hot loop? Could this all be computed when the sampler is constructed instead of being computed every time a ROI is sampled?

zygmuntszpak · 2019-04-20T12:48:04Z

src/tracker/tracker.jl

+        end
+        new(box, sampler, num_of_classifiers, initial_iterations, num_of_features)
+    end
+end


Note that in general you can write inequalities like this in Julia:

if 0 < x < 1 do something end

rather than

if x > 0 && x < 1 do something end

The former is much more readable.

Deepank308 · 2019-04-20T13:13:13Z

Thank you @zygmuntszpak for your suggestions. I will surely inculcate them into the imlpementations but at this very time I'm quite occupied with my End-Semester examinations(starting from the next week). I wanted to ask you whether it is fine if I continue after 30th April?

Apologies for the same.

johnnychen94

I agree with @zygmuntszpak that there're many unnecessary fields declared here, which would make future changes incompatible. We can't say much about BoostingTrackerTargetState and ConfidenceMap at present since they're not used in the implementation.

Here're some of my thoughts.

One rule of thumb that can be useful is occam's razor: don't give the struct too much information and mutability until it's really necessary.

However, it's quite hard to judge in practice. Take the BoxROI as an example,

On one hand, as @zygmuntszpak says, ROI is already self-explained without the content of a concrete image. I see you need img here to check if the ROI is valid for the image but that's not the real need. The intuition of its unnecessity here is: ROI only become invalid until we combine it with an image.

On the other hand, ROI is always an ROI of something, so holding an image as a field of it is also justified.

I don't have any preference for the ROI example, whatever gives better performance wins.

It's absolutely okay to continue it after your exams, nobody in the community will push you, and I've already seen your productivity here and there. 👍 By all means, we do this work with our enthusiasm.

johnnychen94 · 2019-04-20T23:22:44Z

src/tracker/tracker_sampler.jl

@@ -0,0 +1,100 @@
+abstract type AbstractROI end
+mutable struct BoxROI{T <: AbstractArray, S <: AbstractArray} <: AbstractROI


Is img used through the entire tracking life? If the answer is yes, we then have some reason to hold it in some struct( e.g., Tracker or BoxROI). If it's only used for initialization purpose, it's better not to do so.

johnnychen94 · 2019-04-20T23:34:08Z

src/core.jl

+    #Initialized
+    position::SVector{2, Float64}
+    height::Integer
+    width::Integer


I guess this stands for tracking region

position::SVector{2, Float64} height::Integer width::Integer

you can use Range to represent it as well.

Add boosting tracker

1f23e78

johnnychen94 reviewed Apr 4, 2019

View reviewed changes

src/tracker.jl Outdated Show resolved Hide resolved

johnnychen94 reviewed Apr 4, 2019

View reviewed changes

src/tracker.jl Outdated Show resolved Hide resolved

johnnychen94 reviewed Apr 4, 2019

View reviewed changes

src/tracker.jl Outdated Show resolved Hide resolved

Deepank308 added 2 commits April 17, 2019 12:24

Merge remote-tracking branch 'upstream/master' into boosting

252c01e

Finalise tracker API

ed3dc2c

johnnychen94 reviewed Apr 17, 2019

View reviewed changes

Adds tracker sampler and update Project.toml

dca7da3

johnnychen94 reviewed Apr 18, 2019

View reviewed changes

arijitkar98 reviewed Apr 19, 2019

View reviewed changes

src/tracker/tracker.jl Outdated Show resolved Hide resolved

arijitkar98 reviewed Apr 19, 2019

View reviewed changes

johnnychen94 reviewed Apr 19, 2019

View reviewed changes

Deepank308 added 2 commits April 19, 2019 15:10

Add minute changes

3ffa15e

Add tests for ROI sampling

fcddb94

johnnychen94 reviewed Apr 20, 2019

View reviewed changes

src/tracker/tracker_sampler.jl Show resolved Hide resolved

src/tracker/tracker_sampler.jl Show resolved Hide resolved

src/tracker/tracker_sampler.jl Show resolved Hide resolved

johnnychen94 reviewed Apr 20, 2019

View reviewed changes

arijitkar98 reviewed Apr 20, 2019

View reviewed changes

src/tracker/tracker.jl Show resolved Hide resolved

johnnychen94 reviewed Apr 20, 2019

View reviewed changes

zygmuntszpak reviewed Apr 20, 2019

View reviewed changes

johnnychen94 reviewed Apr 21, 2019

View reviewed changes


		abstract type AbstractTrackerTargetState end

		mutable struct BoostingTrackerTargetState <: AbstractTrackerTargetState

		@@ -0,0 +1,100 @@
		abstract type AbstractROI end
		mutable struct BoxROI{T <: AbstractArray, S <: AbstractArray} <: AbstractROI


		CurrentSampler(overlap::F = 0.99, search_factor::F = 1.8) where{F <: AbstractFloat} = CurrentSampler{F}(overlap, search_factor)

		function sample_roi(sampler::CurrentSampler, box::BoxROI)

[WIP]Add boosting tracker #20

Are you sure you want to change the base?

[WIP]Add boosting tracker #20

Conversation

Deepank308 commented Apr 4, 2019 • edited Loading

johnnychen94 commented Apr 4, 2019 • edited Loading

codecov bot commented Apr 4, 2019

Codecov Report

codecov bot commented Apr 4, 2019 • edited Loading

Codecov Report

johnnychen94 commented Apr 4, 2019

Deepank308 commented Apr 10, 2019 • edited Loading

johnnychen94 commented Apr 10, 2019 via email • edited Loading

johnnychen94 commented Apr 10, 2019 • edited Loading

Deepank308 commented Apr 11, 2019 • edited Loading

johnnychen94 commented Apr 11, 2019 • edited Loading

johnnychen94 commented Apr 11, 2019

zygmuntszpak commented Apr 12, 2019

johnnychen94 commented Apr 12, 2019

Deepank308 commented Apr 17, 2019 • edited Loading

Deepank308 commented Apr 18, 2019

johnnychen94 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnychen94 commented Apr 19, 2019

johnnychen94 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygmuntszpak commented Apr 20, 2019

johnnychen94 Apr 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygmuntszpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deepank308 commented Apr 20, 2019

johnnychen94 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deepank308 commented Apr 4, 2019 •

edited

Loading

johnnychen94 commented Apr 4, 2019 •

edited

Loading

codecov bot commented Apr 4, 2019 •

edited

Loading

Deepank308 commented Apr 10, 2019 •

edited

Loading

johnnychen94 commented Apr 10, 2019 via email •

edited

Loading

johnnychen94 commented Apr 10, 2019 •

edited

Loading

Deepank308 commented Apr 11, 2019 •

edited

Loading

johnnychen94 commented Apr 11, 2019 •

edited

Loading

Deepank308 commented Apr 17, 2019 •

edited

Loading

johnnychen94 left a comment •

edited

Loading

johnnychen94 left a comment •

edited

Loading

johnnychen94 Apr 20, 2019 •

edited

Loading

johnnychen94 left a comment •

edited

Loading