fix(patch): Clamp percentiles to actually recorded min/max instead of rounding to equivalent levels (#14)

Clamp percentile result to min/max actual recorded range as we have perfect fidelity there, unexpected p0/p100 otherwise.

Co-authored-by: dimlio <[email protected]>

Author: hassila

Sources/Histogram/Histogram.swift

@@ -326,6 +326,14 @@ public struct Histogram<Count: FixedWidthInteger & Codable>: Codable {
      *            in the histogram are either larger than or equivalent to. Returns 0 if no recorded values exist.
      */
     public func valueAtPercentile(_ percentile: Double) -> UInt64 {
+        guard percentile != 0.0 else {
+            return self.min
+        }
+
+        guard percentile < 100.0 else {
+            return maxRecorded
+        }
+
         // Truncate to 0..100%, and remove 1 ulp to avoid roundoff overruns into next bucket when we
         // subsequently round up to the nearest integer:
         let requestedPercentile = Swift.min(Swift.max(percentile.nextDown, 0.0), 100.0)
@@ -342,9 +350,8 @@ public struct Histogram<Count: FixedWidthInteger & Codable>: Codable {
             totalToCurrentIndex += UInt64(counts[i])
             if totalToCurrentIndex >= countAtPercentile {
                 let valueAtIndex = valueFromIndex(i)
-                return (percentile == 0.0) ?
-                    lowestEquivalentForValue(valueAtIndex) :
-                    highestEquivalentForValue(valueAtIndex)
+                let returnValue = highestEquivalentForValue(valueAtIndex)
+                return Swift.max(Swift.min(returnValue, maxRecorded), self.min)
             }
         }
         return 0
@@ -435,7 +442,7 @@ public struct Histogram<Count: FixedWidthInteger & Codable>: Codable {
     }
 
     /**
-     * Get the lowest recorded value level in the histogram.
+     * Get the lowest recorded value in the histogram.
      *
      * If the histogram has no recorded values, the value returned is undefined.
      *
@@ -446,12 +453,25 @@ public struct Histogram<Count: FixedWidthInteger & Codable>: Codable {
     }
 
     /**
-     * Get the highest recorded value level in the histogram.
+     * Get the highest recorded value in the histogram.
      *
      * If the histogram has no recorded values, the value returned is undefined.
      *
      * - Returns: The maximum value recorded in the histogram.
      */
+    public var maxRecorded: UInt64 {
+        maxValue
+    }
+
+    /**
+     * Get the highest recorded value level in the histogram.
+     *
+     * If the histogram has no recorded values, the value returned is undefined.
+     *
+     * NB this can be larger than the maximum recorded value due to precision.
+     *
+     * - Returns: The maximum value level recorded in the histogram.
+     */
     public var max: UInt64 {
         maxValue == 0 ? 0 : highestEquivalentForValue(maxValue)
     }
@@ -460,6 +480,7 @@ public struct Histogram<Count: FixedWidthInteger & Codable>: Codable {
      * Get the lowest recorded non-zero value level in the histogram.
      *
      * If the histogram has no recorded values, the value returned is undefined.
+     * NB this can be smaller than the minimum recorded value due to precision.
      *
      * - Returns: The lowest recorded non-zero value level in the histogram.
      */
@@ -1347,3 +1368,5 @@ extension Histogram: TextOutputStreamable {
         outputPercentileDistribution(to: &to, outputValueUnitScalingRatio: 1.0)
     }
 }
+
+// swiftlint:enable file_length type_body_length line_length identifier_name

Sources/HistogramExample/HistogramExample.swift

@@ -53,3 +53,5 @@ enum HistogramExample {
         print("\n", histogram)
     }
 }
+
+// swiftlint:enable identifier_name

Tests/HistogramTests/HistogramAutosizingTests.swift

@@ -8,7 +8,7 @@
 // http://www.apache.org/licenses/LICENSE-2.0
 //
 
-// swiftlint:disable identifier_name todo
+// swiftlint:disable todo
 
 @testable import Histogram
 import XCTest
@@ -86,3 +86,5 @@ final class HistogramAutosizingTests: XCTestCase {
         }
     }
 }
+
+// swiftlint:enable todo

Tests/HistogramTests/HistogramDataAccessTests.swift

@@ -491,3 +491,5 @@ final class HistogramDataAccessTests: XCTestCase {
         XCTAssertEqual(4_104, snapshots.count)
     }
 }
+
+// swiftlint:enable file_length identifier_name line_length trailing_comma

Tests/HistogramTests/HistogramTests.swift

@@ -20,6 +20,21 @@ final class HistogramTests: XCTestCase {
     private static let numberOfSignificantValueDigits = SignificantDigits.three
     private static let value: UInt64 = 4
 
+    // This is a check for valueAtPercentiles that would return 999936
+    // instead of 1000_001 for p0 (which would return lowestEquivalentForValue instead of min)
+    // and it would return 1000447 for p100, instead of max
+    func testPercentilesMinMax() throws {
+        var histogram = Histogram<UInt64>(lowestDiscernibleValue: 1, highestTrackableValue: 3_600_000_000, numberOfSignificantValueDigits: .three)
+        for _ in 0 ..< 90 {
+            histogram.record(1_000_001)
+        }
+        XCTAssertEqual(histogram.minNonZeroValue, 1_000_001)
+        XCTAssertEqual(histogram.min, 1_000_001)
+        XCTAssertEqual(histogram.min, histogram.valueAtPercentile(0.0))
+        XCTAssertEqual(histogram.maxRecorded, 1_000_001)
+        XCTAssertEqual(histogram.maxRecorded, histogram.valueAtPercentile(100.0))
+    }
+
     func testCreate() throws {
         let h = Histogram<UInt64>(lowestDiscernibleValue: 1, highestTrackableValue: 3_600_000_000, numberOfSignificantValueDigits: .three)
         XCTAssertEqual(h.counts.count, 23_552)
@@ -504,3 +519,5 @@ final class HistogramTests: XCTestCase {
         XCTAssertEqual(computedMaxValue, h.maxValue)
     }
 }
+
+// swiftlint:enable file_length identifier_name line_length