rfac: Performance improvement to recalculateBins (#113)

* Add performance tests for Histogram method `recalculateBins` and run `swift test --generate-linuxmain`.

* rfac: Faster algorithm for `recalculateBins` which is one of the main culprits in slowing down the tests.

* rfac: Add `isSorted` flag to `HistogramSeries` and improve the performance of `recalculateBins`

This flag is set if `data` is sorted and unset if the user sets the `data` property on the series manually, it is used to choose the fastest algorithm for `recalculateBins`.

* rfac: Clean up calculateSeriesData.

* docs: Add comments explaining both algorithms and their performance.

* chor: val -> value

* rfac: use `max` function and in the second algorithm hoist it out of the loop for better performance. Improve/update comments.

* Attempt to add Big-O notation to the algorithms... is this right?

* doc: add comment

* doc: update comment

* change class back to struct and run `swift test --generate-linuxmain`

* [docs][chor] improve comment on `isSorted` and change `true` to `binIndex < binEndIndex` as suggested in PR#113

* rfac: strip `recalculateBins` down to one algorithm that does not require `data` to be sorted and update tests

* rfac: update tests manifest

* rfac: don't sort the data in the initialiser

* bfix: fix code that took advantage of the data being sorted without actually being sure of that

* docs: Improve comments about workaround

* rfac: Create new test suite for performance tests

Author: odmir

Sources/SwiftPlot/Histogram.swift

@@ -90,11 +90,25 @@ extension Histogram: HasGraphLayout {
         var results = DrawingData()
         var markers = PlotMarkers()
 
-        var minimumX = histogramSeries.data.first!
-        var maximumX = histogramSeries.data.last!
+        // FIXME: Handle `histogramSeries.data` being empty.
+        // TODO: (Performance) Fix this because we are going through
+        // the data twice right now for each series we have...
+        /// We were depending on `histogram.data` being sorted to get the `minimumX` and `maximumX`
+        /// by getting the `.first`and `.last` element. This led to bugs down the line where it is expected
+        /// that `minimumX < maximumX`. So this is the workaround for now.
+        ///
+        /// This was added as part of PR #113
+        guard var minimumX = histogramSeries.data.min(),
+            var maximumX = histogramSeries.data.max() else {
+                fatalError("Histogram: (temporary) We are not handling the case where the user supplied an empty array.")
+        }
+        
         for series in histogramStackSeries {
-            minimumX = min(minimumX, series.data.first!)
-            maximumX = max(maximumX, series.data.last!)
+            guard let minX = series.data.min(), let maxX = series.data.max() else {
+                    fatalError("Histogram: (temporary) We are not handling the case where the user supplied an empty array.")
+            }
+            minimumX = min(minimumX, minX)
+            maximumX = max(maximumX, maxX)
         }
         minimumX = T(roundFloor10(Float(minimumX)))
         maximumX = T(roundCeil10(Float(maximumX)))
@@ -308,31 +322,53 @@ extension Histogram: HasGraphLayout {
 
 private extension Histogram {
 
+    /// If the data is not sorted, run through each value in `data`, binary search the right bin and increment its frequency.
+    ///
+    /// Performance: O(n log(m)). n: `data.count`, m: `binFrequency.count`.
+    ///   This algorithm runs through `data` once and for each value in `data` a binary search is performed on the bins' lower x limits.
+    ///   It runs through `binFrequency` array once to get the maximum frequency.
+    ///   It get/sets `binFrequency` value `data.count` amount of times.
     func recalculateBins(series: HistogramSeries<T>,
                          binStart: T,
                          binEnd: T,
                          binInterval: T) -> (binFrequency: [Float], maxFrequency: Float) {
-        
-        var maximumFrequency = Float(0)
-        var binFrequency = stride(from: Float(binStart), through: Float(binEnd), by: Float(binInterval)).map {
-            start -> Float in
-            let end = start + Float(binInterval)
-            var count: Float = 0
-            for d in series.data {
-                if(d < T(end) && d >= T(start)) {
-                    count += 1
+        var binFrequency = [Float](repeating: 0.0, count: series.bins)
+        let lastIndex = binFrequency.endIndex - 1
+        for value in series.data {
+            var start = 0
+            var end = lastIndex
+            var current = start + (end - start) / 2
+            while end - start > 1 {
+                if value >= binStart + T(current) * binInterval {
+                    start = current
+                } else {
+                    end = current
                 }
+                current = start + (end - start) / 2
             }
-            maximumFrequency = max(count, maximumFrequency)
-            return count
+            
+            binFrequency[current] += 1
         }
+        var maximumFrequency = binFrequency.max() ?? 0.0
+        
         if (isNormalized) {
             let factor = Float(series.data.count)*Float(binInterval)
-            for index in 0..<binFrequency.count {
+            for index in 0..<series.bins {
                 binFrequency[index]/=factor
             }
             maximumFrequency/=factor
         }
         return (binFrequency, maximumFrequency)
     }
 }
+
+// For testing private methods.
+
+extension Histogram {
+    func testRecalculateBins(series: HistogramSeries<T>,
+                             binStart: T,
+                             binEnd: T,
+                             binInterval: T) {
+        let (_, _) = recalculateBins(series: series, binStart: binStart, binEnd: binEnd, binInterval: binInterval)
+    }
+}

Sources/SwiftPlot/HistogramSeries.swift

@@ -10,7 +10,7 @@ public struct HistogramSeries<T> where T: Comparable {
                 label: String,
                 color: Color,
                 histogramType: HistogramSeriesOptions.HistogramType) {
-        self.data = data.sorted()
+        self.data = data
         self.bins = bins
         self.label = label
         self.color = color

Tests/SwiftPlotTests/PerformanceTests/performance-histogram.swift

@@ -0,0 +1,22 @@
+import XCTest
+@testable import SwiftPlot
+//import SVGRenderer
+//#if canImport(AGGRenderer)
+//import AGGRenderer
+//#endif
+//#if canImport(QuartzRenderer)
+//import QuartzRenderer
+//#endif
+
+extension PerformanceTests {
+    
+    /// Performance tests for the `Histogram.recalculateBins`.
+    func testPerformanceHistogramRecalculateBins() throws {
+        let histogram = Histogram<Float>(isNormalized: false, enableGrid: false)
+        
+        let histogramSeries = HistogramSeries(data: histogram_step_values, bins: 50, label: "HISTOGRAM PERFORMANCE `recalculateBins`", color: .black, histogramType: .bar)
+        measure {
+            histogram.testRecalculateBins(series: histogramSeries, binStart: 40, binEnd: 160, binInterval: (160-40)/Float(histogram.histogramSeries.bins))
+        }
+    }
+}

Tests/SwiftPlotTests/PerformanceTests/performanceTests.swift

@@ -0,0 +1,3 @@
+import XCTest
+
+final class PerformanceTests: SwiftPlotTestCase {}

Tests/SwiftPlotTests/XCTestManifests.swift

@@ -17,8 +17,8 @@ extension AnnotationTests {
     static let __allTests__AnnotationTests = [
         ("testAnnotationArrow", testAnnotationArrow),
         ("testAnnotationArrowDart", testAnnotationArrowDart),
-        ("testAnnotationArrowWedge", testAnnotationArrowWedge),
         ("testAnnotationArrowDoubleHeaded", testAnnotationArrowDoubleHeaded),
+        ("testAnnotationArrowWedge", testAnnotationArrowWedge),
         ("testAnnotationText", testAnnotationText),
         ("testAnnotationTextBoundingBox", testAnnotationTextBoundingBox),
     ]
@@ -51,6 +51,7 @@ extension HistogramTests {
     static let __allTests__HistogramTests = [
         ("testHistogram", testHistogram),
         ("testHistogramMultiStacked", testHistogramMultiStacked),
+        ("testHistogramMultiStackedColorBleed", testHistogramMultiStackedColorBleed),
         ("testHistogramMultiStackedStep", testHistogramMultiStackedStep),
         ("testHistogramStacked", testHistogramStacked),
         ("testHistogramStackedStep", testHistogramStackedStep),
@@ -85,6 +86,15 @@ extension LineChartTests {
     ]
 }
 
+extension PerformanceTests {
+    // DO NOT MODIFY: This is autogenerated, use:
+    //   `swift test --generate-linuxmain`
+    // to regenerate.
+    static let __allTests__PerformanceTests = [
+        ("testPerformanceHistogramRecalculateBins", testPerformanceHistogramRecalculateBins),
+    ]
+}
+
 extension ScatterPlotTests {
     // DO NOT MODIFY: This is autogenerated, use:
     //   `swift test --generate-linuxmain`
@@ -110,6 +120,7 @@ public func __allTests() -> [XCTestCaseEntry] {
         testCase(BarchartTests.__allTests__BarchartTests),
         testCase(HistogramTests.__allTests__HistogramTests),
         testCase(LineChartTests.__allTests__LineChartTests),
+        testCase(PerformanceTests.__allTests__PerformanceTests),
         testCase(ScatterPlotTests.__allTests__ScatterPlotTests),
         testCase(SubPlotTests.__allTests__SubPlotTests),
     ]