Implemented Moving Histogram algorithm

[spran180]

Fixes #752

Summary
This PR implemented Moving Histogram Algorithm as stated in #752. This change includes the addition of a new class 'MovHistogram' and its associated unit tests.

[gavv]

Hi, I see it's draft and haven't look at the code closely, but here a couple of preliminary comments:

• PRs should be targeted to develop branch, not master (see here)
• Please follow code style (see here)
• Template classes should be defined in header, not in .cpp

[spran180]

Hi @gavv,

Thanks for reviewing my code. I will correct it shortly as per your suggestions.

[spran180]

Hello @gavv,

I have addressed the issues you highlighted. The branch has been rebased to develop, and the code has been revised to align with the coding style. Everything should be in order now. Kindly inform me if any further changes or checks are needed.

[gavv]

Thanks for PR! A couple of comments from me as well.

[spran180]

Thank you for the review, @adrianopol and @gavv. I will look into it and update the code accordingly.

[spran180]

Hey @adrianopol, I'm using pair<T, T> to represent the value_range {min_val, max_val} instead of creating separate variables. Is this approach correct?

[gavv]

Hey @adrianopol, I'm using pair<T, T> to represent the value_range {min_val, max_val} instead of creating separate variables. Is this approach correct?

We don't use STL containers, so please use separate arguments. See here: https://roc-streaming.org/toolkit/docs/development/coding_guidelines.html

[spran180]

Hey @gavv / @adrianopol,

I've addressed the changes that you've requested pls go through it once and let me know if any further changes are required.

[adrianopol]

@spran180 , thank you for implementation.

We also need more test cases here, for example:

• T = float
• min and max < 0
• win_length == 1

They may be added in a different PR if you wish.

[gavv]

@spran180 , thank you for implementation.

We also need more test cases here, for example:

* T = float

* min and max < 0

* win_length == 1
  They may be added in a different PR if you wish.

I agree.

Also, current tests don't cover a few important aspects:

• when there are multiple values in every bin (currently every bin is either 0 or 1)
• how bin counters increment and decrement when rolling window is moved (e.g. you call add_value, some bin changes from N to N-1, and another bin changes from K to K+1)
• out of bounds values (clamping)

@spran180 Let me know if you would like to add these tests or it's better to create a new issue for that.

[spran180]

@spran180 , thank you for implementation.
We also need more test cases here, for example:

* T = float

* min and max < 0

* win_length == 1
  They may be added in a different PR if you wish.

I agree.

Also, current tests don't cover a few important aspects:

• when there are multiple values in every bin (currently every bin is either 0 or 1)
• how bin counters increment and decrement when rolling window is moved (e.g. you call add_value, some bin changes from N to N-1, and another bin changes from K to K+1)
• out of bounds values (clamping)

@spran180 Let me know if you would like to add these tests or it's better to create a new issue for that.

Yes, I believe I should create a separate PR to add tests.

[gavv]

The build failure was docker hub timeout, not related to the PR. I've restarted the build and it's gone.

Yes, I believe I should create a separate PR to add tests.

@spran180 Great, do you plan to work on it, or do I create an issue for the future?

[spran180]

The build failure was docker hub timeout, not related to the PR. I've restarted the build and it's gone.

Yes, I believe I should create a separate PR to add tests.

@spran180 Great, do you plan to work on it, or do I create an issue for the future?

Indeed, I am interested in working on it.

[gavv]

Thanks for PR!

Indeed, I am interested in working on it.

Perfect. I've copied Andrew's and mine comments here: #752 (comment)