[v2,5/5] libipa: histogram: Fix interQuantileMean() for small ranges
diff mbox series

Message ID 20250401123633.58887-6-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Fix histogram for some (corner) cases
Related show

Commit Message

Stefan Klug April 1, 2025, 12:36 p.m. UTC 
The interQuantileMean() is supposed to return a weighted mean value
between two quantiles. This works for fine histograms, but fails for
coarse histograms and small quantile ranges because the weight is always
taken from the lower border of the bin.

Fix that by rewriting the algorithm to calculate a lower and upper bound
for every (partial) bin that goes into the mean calculation and weight
the bins by the middle of these bounds.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---

Changes in v2:
- Order fix patch after test patch
- Remove should_fail
- Added documentation based on review
- Improved code style based on review
---
 src/ipa/libipa/histogram.cpp | 39 +++++++++++++++++++++++-------------
 test/ipa/libipa/meson.build  |  2 +-
 2 files changed, 26 insertions(+), 15 deletions(-)

Patch
diff mbox series 

diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp
index ea042f0a17b9..bcf263908236 100644
--- a/src/ipa/libipa/histogram.cpp
+++ b/src/ipa/libipa/histogram.cpp
@@ -149,26 +149,37 @@  double Histogram::quantile(double q, uint32_t first, uint32_t last) const
 double Histogram::interQuantileMean(double lowQuantile, double highQuantile) const
 {
 	ASSERT(highQuantile > lowQuantile);
-	/* Proportion of pixels which lies below lowQuantile */
-	double lowPoint = quantile(lowQuantile);
-	/* Proportion of pixels which lies below highQuantile */
-	double highPoint = quantile(highQuantile, static_cast<uint32_t>(lowPoint));
-	double sumBinFreq = 0, cumulFreq = 0;
-
-	for (double p_next = floor(lowPoint) + 1.0;
-	     p_next <= ceil(highPoint);
-	     lowPoint = p_next, p_next += 1.0) {
-		int bin = floor(lowPoint);
+
+	/* Proportion of pixels which lies below lowQuantile and highQuantile. */
+	const double lowPoint = quantile(lowQuantile);
+	const double highPoint = quantile(highQuantile, static_cast<uint32_t>(lowPoint));
+
+	double sumBinFreq = 0;
+	double cumulFreq = 0;
+
+	/*
+         * Calculate the mean pixel value between the low and high points by
+         * summing all the pixels between the two points, and dividing the sum
+         * by the number of pixels. Given the discrete nature of the histogram
+         * data, the sum of the pixels is approximated by accumulating the
+         * product of the bin values (calculated as the mid point of the bin) by
+         * the number of pixels they contain, for each bin in the internal.
+         */
+	for (unsigned bin = std::floor(lowPoint); bin < std::ceil(highPoint); bin++) {
+		const double lowBound = std::max<double>(bin, lowPoint);
+		const double highBound = std::min<double>(bin + 1, highPoint);
+
 		double freq = (cumulative_[bin + 1] - cumulative_[bin])
-			* (std::min(p_next, highPoint) - lowPoint);
+			    * (highBound - lowBound);
 
 		/* Accumulate weighted bin */
-		sumBinFreq += bin * freq;
+		sumBinFreq += (highBound + lowBound) / 2 * freq;
+
 		/* Accumulate weights */
 		cumulFreq += freq;
 	}
-	/* add 0.5 to give an average for bin mid-points */
-	return sumBinFreq / cumulFreq + 0.5;
+
+	return sumBinFreq / cumulFreq;
 }
 
 } /* namespace ipa */
diff --git a/test/ipa/libipa/meson.build b/test/ipa/libipa/meson.build
index 83c84bd8c227..8c63ebd8e2f7 100644
--- a/test/ipa/libipa/meson.build
+++ b/test/ipa/libipa/meson.build
@@ -2,7 +2,7 @@ 
 
 libipa_test = [
     {'name': 'fixedpoint', 'sources': ['fixedpoint.cpp']},
-    {'name': 'histogram', 'sources': ['histogram.cpp'], 'should_fail': true},
+    {'name': 'histogram', 'sources': ['histogram.cpp']},
     {'name': 'interpolator', 'sources': ['interpolator.cpp']},
 ]