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

Add histogram class and related functionality #499

Merged
merged 31 commits into from
Jan 23, 2021

Conversation

codejaeger
Copy link
Contributor

@codejaeger codejaeger commented May 29, 2020

Description

This contains the initial implementation of histogram functionality for STL containers for GIL images. Basic tests checking for correctness of histogram have been provided. Usage syntax and distinction between compatible and incompatible containers have been illustrated in a example file.

There was some dicussion regarding the name of the function (which now is image_histogram) in the mailing thread (link). I chose on 'image_histogram' since the name 'histogram' is used as the class name in Boost.Histogram and might cause problems in namespace mix-up. Another option could be histogram_1d since these functions are meant for that purpose.

Dependency

Optional - Docs for the this implementation are running in a parallel PR #503.

References

Discussions on boost-gil mailing threads.

Tasklist

  • Add test case(s)
  • Provide documentation
  • Ensure all CI builds pass - R.I.P. Travis CI
  • Review and approve

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.

I have some objections with the code format(template parameters) I don't find the really appealing to the eyes but I maybe wrong. @mloskot what are your suggestions on it!

include/boost/gil/histogram.hpp Outdated Show resolved Hide resolved
include/boost/gil/histogram.hpp Outdated Show resolved Hide resolved
include/boost/gil/histogram.hpp Outdated Show resolved Hide resolved
include/boost/gil/histogram.hpp Outdated Show resolved Hide resolved
include/boost/gil/histogram.hpp Outdated Show resolved Hide resolved
test/core/histogram/CMakeLists.txt Outdated Show resolved Hide resolved
test/core/histogram/Jamfile Outdated Show resolved Hide resolved
example/histogram_impl.cpp Outdated Show resolved Hide resolved
include/boost/gil/histogram.hpp Outdated Show resolved Hide resolved
 * Shifted type traits to gil/concepts/detail/histogram.hpp

 * Added binning logic for unmutable container

 * Added missing endline, changed parameter name etc.
@codejaeger codejaeger requested a review from lpranam June 2, 2020 09:49
@codejaeger
Copy link
Contributor Author

codejaeger commented Jun 2, 2020

I have ideas I would like to share further on the current approach.
I added a diagram to explain better

histogram

This approach might ease extending the implementation design in the future to support both multi dimensional histograms and external containers.
In brief, I chose this approach because

  • it will simplify the usage for the end user (just deal with the simple function call syntax)
  • handle cases like when a call is made to histogram_2d with a specific container type but the image provided is gray scale so the underlying implementation should call histogram_1d_impl
  • simplify providing extensions of other container types, at most they need to replicate the procedure if not use the already present implementation

So basically the first overloads make sure appropriate type checks are performed on both image and container type. The second overload would choose to call the overload specific to the requirements and the third overload can be easily extended to support higher dimensional containers (of any type).
I would like to know if this seems right or there will be some unwanted implications of using this approach.

@mloskot mloskot added google-summer-of-code All items related to GSoC activities cat/feature New feature or functionality labels Jun 5, 2020
@codejaeger codejaeger changed the title Test implementation of histogram functionality [WIP]: Test implementation of histogram functionality Jun 12, 2020
@codejaeger codejaeger marked this pull request as draft June 12, 2020 08:09
A new histogram class proposed with close suuport for gil
image constructs.

Code Refactor :

 - Shift current stl implmentation to extension to
   serve as example for overloading fill_histogram
@codejaeger codejaeger marked this pull request as ready for review June 12, 2020 18:38
@codejaeger codejaeger requested a review from mloskot June 12, 2020 18:38
@codejaeger codejaeger changed the title [WIP]: Test implementation of histogram functionality Test implementation of histogram functionality Jun 14, 2020
@codejaeger codejaeger changed the title Test implementation of histogram functionality [WIP]: Test implementation of histogram functionality Jun 16, 2020
@codejaeger codejaeger marked this pull request as draft June 16, 2020 22:05
@codejaeger codejaeger force-pushed the histogram_impl branch 6 times, most recently from b0963c8 to 6d7697e Compare June 18, 2020 14:02
@codejaeger codejaeger marked this pull request as draft July 28, 2020 19:50
 - Update helper func tuple_compare and functor filler

 - Update member func fill

 - Update global func fill_histogram
@codejaeger codejaeger force-pushed the histogram_impl branch 2 times, most recently from 3fa0941 to 27c9351 Compare July 30, 2020 20:29
 - Separate histogram fill tests from utilities

 - Add helper impl tuple limit
@codejaeger
Copy link
Contributor Author

@mloskot , @lpranam I have

  1. updated the example file for histogram
  2. reduced the warnings from the tests and example
  3. Added Doxygen comments for the new changes
    Unless the CI checks fail it is ready for review now.

@codejaeger codejaeger requested a review from lpranam July 31, 2020 09:28
@codejaeger codejaeger marked this pull request as ready for review July 31, 2020 09:29
@mloskot
Copy link
Member

mloskot commented Aug 7, 2020

@codejaeger Thanks for the update.

Something is wrong with the histogram example, see https://ci.appveyor.com/project/stefanseefeld/gil/builds/34407783/job/fw3u0ba7yno9rdui

The last build job of AppVeyor is CMake-based which builds examples too.

@codejaeger
Copy link
Contributor Author

Thanks @mloskot for pointing out the bug. I hope it gets fixed with the last commit.

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.

I don't have any major issues to report about this PR, so I'm happy to approve it.
The only 'unresolved' comments are about the x<space><space>=<space>y formatting due to use of the clang-format and pointed out by @lpranam which I'd consider not a stopper issue that can be addressed after the merge along with other updates or improvements that are applicable post-merge.

@codejaeger Once again, thanks for your contributions

@mloskot mloskot added this to the Boost 1.76+ milestone Jan 22, 2021
@mloskot mloskot merged commit 3e729e5 into boostorg:develop Jan 23, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
A new histogram class proposed with close suport for gil
image constructs.

Shift the stl support implmentation to extension to
serve as example for overloading fill_histogram.

Add cumulative histogram and histogram normalization.

Co-authored-by: debabrata1 <debabrata@goodhealthapp.com>
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 22, 2021
A new histogram class proposed with close suport for gil
image constructs.

Shift the stl support implmentation to extension to
serve as example for overloading fill_histogram.

Add cumulative histogram and histogram normalization.

Co-authored-by: debabrata1 <debabrata@goodhealthapp.com>
@mloskot mloskot mentioned this pull request May 12, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/feature New feature or functionality google-summer-of-code All items related to GSoC activities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants