Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Functionality for Histogram of Oriented Gradients #597

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Sayan-Chaudhuri
Copy link

@Sayan-Chaudhuri Sayan-Chaudhuri commented Apr 18, 2021

Currently there are no feature descriptors in boost gil . Histogram of Oriented Gradients is a very popular feature descriptor that has been applied successfully for tasks such as pedestrian detection in images and videos. The current implementation follows from the same in scikit-image library.
Reference Paper- Dalal, N and Triggs, B, Histograms of Oriented Gradients for Human Detection, IEEE Computer Society Conference on Computer Vision and Pattern
recognition 2005 San Diego, CA, USA, https://lear.inrialpes.fr/people/triggs/pubs/Dalal-cvpr05.pdf,:DOI:`10.1109/CVPR.2005.177`

Description

References

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

Currently there are no feature descriptors in boost gil . Histogram of Oriented Gradients is a very popular feature descriptor that has been applied successfully for tasks such as  pedestrian detection in images and videos. The current implementation follows from the same in scikit-image library.
Reference Paper- Dalal, N and Triggs, B, Histograms of Oriented Gradients for Human Detection, IEEE Computer Society Conference on Computer Vision and Pattern 
                               recognition    2005 San Diego, CA, USA, https://lear.inrialpes.fr/people/triggs/pubs/Dalal-cvpr05.pdf,:DOI:`10.1109/CVPR.2005.177`
@codejaeger
Copy link
Contributor

@Sayan-Chaudhuri you might want to take a look at the docs to know some of the intrinsic details about GIL. It had helped me quite a lot. Cheers!

@Sayan-Chaudhuri
Copy link
Author

@codejaeger Thanks for your feedback. I have already gone through the docs. If you can kindly say, whether you found anything wrong that can be corrected by reading the docs, otherwise I am unable to understand the issue you are referring to.

@codejaeger
Copy link
Contributor

Oh no, nothing seems wrong. I had thought you did not find the docs since there is a section on image gradients and how to do it faster using native GIL constructs. I also made some comments at the relevant portions of the PR to clarify better.

Copy link
Member

@lpranam lpranam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NO. You can't just copy the code!!! Even if everything is open source there are copyrights and license issue you just can't copy the code. Even if there are no issues of copyright or license why would we want to copy the code!? if I wanted to use scikit code I would have used scikit and not GIL.

There comes many issues with copied code, the first is maintenance cost. No one knows how it works so if something goes wrong we have to spend a significant amount of time solving the problem. Reading code is harder than writing code.

another issue comes in the performance, most codes are written in such a way that it is very optimised in terms of performance for their own codebase. if you just copy it they may not perform as good in GIL as they do in scikit.

another problem is with format and code conventions, code is miserably formatted, template parameters are meaninglessly named, inconsistent spacing and blank lines, overall a mess in terms of code readability

most importantly scikits license does not seem to be compatible with boost (not an expert in legal matters). they seem to be more restricted than boost so definitely, we can't use their code in boost

and the list would go on why not to copy code and especially why not to copy a huge chunk of code...

@Sayan-Chaudhuri
Copy link
Author

@lpranam @codejaeger thanks for your valuable feedback

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hog_implementation.hpp is a bad name

  • don't mix lower/upper case
  • word implementation is redundant

I also suspect spacebar in your keyboard may be broken :-)

@Sayan-Chaudhuri
Copy link
Author

NO. You can't just copy the code!!! Even if everything is open source there are copyrights and license issue you just can't copy the code. Even if there are no issues of copyright or license why would we want to copy the code!? if I wanted to use scikit code I would have used scikit and not GIL.

There comes many issues with copied code, the first is maintenance cost. No one knows how it works so if something goes wrong we have to spend a significant amount of time solving the problem. Reading code is harder than writing code.

another issue comes in the performance, most codes are written in such a way that it is very optimised in terms of performance for their own codebase. if you just copy it they may not perform as good in GIL as they do in scikit.

another problem is with format and code conventions, code is miserably formatted, template parameters are meaninglessly named, inconsistent spacing and blank lines, overall a mess in terms of code readability

most importantly scikits license does not seem to be compatible with boost (not an expert in legal matters). they seem to be more restricted than boost so definitely, we can't use their code in boost

and the list would go on why not to copy code and especially why not to copy a huge chunk of code...

No copyright issues now

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #597 (476a571) into develop (6e91e4b) will increase coverage by 0.13%.
The diff coverage is n/a.

❗ Current head 476a571 differs from pull request most recent head 64f25fe. Consider uploading reports for the commit 64f25fe to get more accurate results

@@             Coverage Diff             @@
##           develop     #597      +/-   ##
===========================================
+ Coverage    78.59%   78.72%   +0.13%     
===========================================
  Files          117      118       +1     
  Lines         5003     5034      +31     
===========================================
+ Hits          3932     3963      +31     
  Misses        1071     1071              

@Sayan-Chaudhuri
Copy link
Author

Hog_implementation.hpp is a bad name

  • don't mix lower/upper case
  • word implementation is redundant

I also suspect spacebar in your keyboard may be broken :-)

I have now referred to the codes of the existing algorithms in GIL library and have tried to format mine in the same way as those.

@Sayan-Chaudhuri
Copy link
Author

@codejaeger Thanks for your valuable suggestion. I am now experimenting with the histogram coded by you. However, I have a question that as per my understanding, you have used an unordered map to build the histogram . The time complexity of an unordered map is O(n). Wont then my program be slower compared to me using N-Dimensional arrays as histograms where time complexity of access is almost O(1)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants