From 302b1cc95a63b4bf7d6f57c393c7ea9e9b2bed29 Mon Sep 17 00:00:00 2001
From: Pranav Sharma <83678994+spran180@users.noreply.github.com>
Date: Mon, 22 Jul 2024 11:24:12 +0530
Subject: [PATCH] addresed requested changes

---
 src/internal_modules/roc_core/mov_histogram.h | 45 ++++++++++++-------
 src/tests/roc_core/test_mov_histogram.cpp     | 18 +++++---
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/src/internal_modules/roc_core/mov_histogram.h b/src/internal_modules/roc_core/mov_histogram.h
index ed2693401..506466454 100644
--- a/src/internal_modules/roc_core/mov_histogram.h
+++ b/src/internal_modules/roc_core/mov_histogram.h
@@ -22,23 +22,30 @@ namespace core {
 
 /// @brief
 //! Efficiently implements moving histogram based on approach
-/// @tparam T
+/// @tparam T The type of values to be stored in the histogram
 template <typename T> class MovHistogram {
 public:
     //! Initialize.
-    MovHistogram(IArena& arena, T value_range, size_t num_bins, size_t window_length)
-        : value_range_(value_range)
+    MovHistogram(IArena& arena,
+                 T value_range_min,
+                 T value_range_max,
+                 size_t num_bins,
+                 size_t window_length)
+        : value_range_min_(value_range_min)
+        , value_range_max_(value_range_max)
         , num_bins_(num_bins)
         , window_length_(window_length)
-        , bin_width(static_cast<T>(value_range) / static_cast<T>(num_bins))
         , ring_buffer_(arena, window_length)
         , bins_(arena)
         , valid_(false) {
-        if (num_bins == 0 || window_length_ == 0) {
+        if (num_bins == 0 || window_length == 0 || value_range_min >= value_range_max) {
             roc_panic(
                 "mov histogram: number of bins and window length must be greater than 0");
         }
 
+        bin_width_ = (static_cast<T>(value_range_max - value_range_min)
+                      / static_cast<T>(num_bins));
+
         if (!ring_buffer_.is_valid() || !bins_.resize(num_bins)) {
             return;
         }
@@ -54,9 +61,9 @@ template <typename T> class MovHistogram {
     //! Add a value to the histogram.
     void add_value(const T& value) {
         if (ring_buffer_.size() == window_length_) {
-            T oldest_bin_ = ring_buffer_.front();
+            T oldest_value = ring_buffer_.front();
             ring_buffer_.pop_front();
-            int oldest_bin_index = getBinIndex(oldest_bin_);
+            size_t oldest_bin_index = get_bin_index_(oldest_value);
             if (oldest_bin_index >= 0
                 && static_cast<size_t>(oldest_bin_index) < num_bins_) {
                 bins_[static_cast<size_t>(oldest_bin_index)]--;
@@ -64,33 +71,39 @@ template <typename T> class MovHistogram {
         }
 
         ring_buffer_.push_back(value);
-        int new_bin_index = getBinIndex(value);
+        size_t new_bin_index = get_bin_index_(value);
         if (new_bin_index >= 0 && static_cast<size_t>(new_bin_index) < num_bins_) {
             bins_[static_cast<size_t>(new_bin_index)]++;
         }
     }
 
     //! returns the number of values in the bin
-    int getBinCounter(size_t bin_index) const {
+    size_t get_bin_counter(size_t bin_index) const {
         return bins_[bin_index];
     }
 
 private:
     //! returns the bin index for a given value
-    int getBinIndex(const T& value) const {
-        if (value > value_range_) {
-            return -1;
+    size_t get_bin_index_(const T& value) const {
+
+        T clamped_value = value;
+
+        if (clamped_value < value_range_min_) {
+            clamped_value = value_range_min_;
+        } else if (clamped_value > value_range_max_) {
+            clamped_value = value_range_max_;
         }
 
-        return static_cast<int>(value / bin_width);
+        return static_cast<size_t>((clamped_value - value_range_min_) / bin_width_);
     }
 
-    T value_range_;
+    T value_range_min_;
+    T value_range_max_;
     size_t num_bins_;
     size_t window_length_;
-    T bin_width;
+    T bin_width_;
     RingQueue<T> ring_buffer_;
-    Array<int> bins_;
+    Array<size_t> bins_;
     bool valid_;
 };
 
diff --git a/src/tests/roc_core/test_mov_histogram.cpp b/src/tests/roc_core/test_mov_histogram.cpp
index 2d42a2e06..7775b1cab 100644
--- a/src/tests/roc_core/test_mov_histogram.cpp
+++ b/src/tests/roc_core/test_mov_histogram.cpp
@@ -47,11 +47,13 @@ TEST_GROUP(movhistogram) {
 };
 
 TEST(movhistogram, single_pass) {
-    const size_t value_range = 100;
+    const size_t value_range_min = 0;
+    const size_t value_range_max = 100;
     const size_t num_bins = 10;
     const size_t win_length = 10;
 
-    MovHistogram<size_t> hist(arena, value_range, num_bins, win_length);
+    MovHistogram<size_t> hist(arena, value_range_min, value_range_max, num_bins,
+                              win_length);
     CHECK(hist.is_valid());
 
     size_t values[win_length];
@@ -61,26 +63,28 @@ TEST(movhistogram, single_pass) {
     }
 
     for (size_t i = 0; i < num_bins; ++i) {
-        LONGS_EQUAL(1, hist.getBinCounter(i));
+        LONGS_EQUAL(1, hist.get_bin_counter(i));
     }
 }
 
 TEST(movhistogram, rolling_window) {
-    const size_t value_range = 100;
+    const size_t value_range_min = 0;
+    const size_t value_range_max = 100;
     const size_t num_bins = 10;
     const size_t win_length = 5;
 
-    MovHistogram<size_t> hist(arena, value_range, num_bins, win_length);
+    MovHistogram<size_t> hist(arena, value_range_min, value_range_max, num_bins,
+                              win_length);
     CHECK(hist.is_valid());
 
     size_t values[win_length * 2];
     for (size_t i = 0; i < win_length * 2; i++) {
-        values[i] = i * (value_range / num_bins);
+        values[i] = i * (value_range_max / num_bins);
         hist.add_value(values[i]);
     }
 
     for (size_t i = 0; i < num_bins; ++i) {
-        LONGS_EQUAL(i < win_length ? 0 : 1, hist.getBinCounter(i));
+        LONGS_EQUAL(i < win_length ? 0 : 1, hist.get_bin_counter(i));
     }
 }