search

roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.09k stars 213 forks source link

Implemented Moving Histogram algorithm #760

Closed spran180 closed 4 months ago

spran180 commented 4 months ago

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.

github-actions[bot] commented 4 months ago

:robot: Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.

gavv commented 4 months ago

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

spran180 commented 4 months ago

Hi @gavv,

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

spran180 commented 4 months ago

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.

spran180 commented 4 months ago

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

spran180 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

gavv commented 4 months ago

@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:

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

spran180 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Thanks for PR!

Indeed, I am interested in working on it.

Perfect. I've copied Andrew's and mine comments here: https://github.com/roc-streaming/roc-toolkit/issues/752#issuecomment-2254591934