Deepank308 commented 5 years ago

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?

johnnychen94 commented 5 years ago

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).

codecov[bot] commented 5 years ago

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%)`	:arrow_down:

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[bot] commented 5 years ago

Codecov Report

:exclamation: 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.

johnnychen94 commented 5 years ago

@Deepank308 Since you're implementing from scratch, I'd like to get feedback from you on https://github.com/zygmuntszpak/ImageBinarization.jl/issues/26

Deepank308 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

zygmuntszpak commented 5 years ago

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 commented 5 years ago

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.

Deepank308 commented 5 years ago

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 commented 5 years ago

I will add tests for sampling in the next commit

johnnychen94 commented 5 years ago

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

zygmuntszpak commented 5 years ago

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

Deepank308 commented 5 years ago

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.

JuliaImages / ImageTracking.jl

[WIP]Add boosting tracker #20

Codecov Report

Codecov Report