Fix Univariate histogram filling

CHANGELOG.md

@@ -50,6 +50,7 @@
 
 ### Fixed
 - Ring inherits RingOperations, not GroupOperations
+- Univariate histogram filling
 
 ### Security
 

build.gradle.kts

@@ -17,7 +17,7 @@ allprojects {
     }
 
     group = "space.kscience"
-    version = "0.3.0-dev-13"
+    version = "0.3.0-dev-14"
 }
 
 subprojects {

kmath-histograms/src/jvmMain/kotlin/space/kscience/kmath/histogram/TreeHistogramSpace.kt

@@ -35,51 +35,56 @@ public class TreeHistogram(
     override val bins: Collection<UnivariateBin> get() = binMap.values
 }
 
+@OptIn(UnstableKMathAPI::class)
+private class TreeHistogramBuilder(val binFactory: (Double) -> UnivariateDomain) : UnivariateHistogramBuilder {
+
+    private class BinCounter(val domain: UnivariateDomain, val counter: Counter<Double> = Counter.real()) :
+        ClosedFloatingPointRange<Double> by domain.range
+
+    private val bins: TreeMap<Double, BinCounter> = TreeMap()
+
+    fun get(value: Double): BinCounter? = bins.getBin(value)
+
+    fun createBin(value: Double): BinCounter {
+        val binDefinition = binFactory(value)
+        val newBin = BinCounter(binDefinition)
+        synchronized(this) { bins[binDefinition.center] = newBin }
+        return newBin
+    }
+
+    /**
+     * Thread safe put operation
+     */
+    override fun putValue(at: Double, value: Double) {
+        (get(at) ?: createBin(at)).apply {
+            counter.add(value)
+        }
+    }
+
+    override fun putValue(point: Buffer<Double>, value: Number) {
+        require(point.size == 1) { "Only points with single value could be used in univariate histogram" }
+        putValue(point[0], value.toDouble())
+    }
+
+    fun build(): TreeHistogram {
+        val map = bins.mapValuesTo(TreeMap<Double, UnivariateBin>()) { (_, binCounter) ->
+            val count = binCounter.counter.value
+            UnivariateBin(binCounter.domain, count, sqrt(count))
+        }
+        return TreeHistogram(map)
+    }
+}
+
 /**
  * A space for univariate histograms with variable bin borders based on a tree map
  */
 @UnstableKMathAPI
 public class TreeHistogramSpace(
-    public val binFactory: (Double) -> UnivariateDomain,
+    private val binFactory: (Double) -> UnivariateDomain,
 ) : Group<UnivariateHistogram>, ScaleOperations<UnivariateHistogram> {
 
-    private class BinCounter(val domain: UnivariateDomain, val counter: Counter<Double> = Counter.real()) :
+    public fun fill(block: UnivariateHistogramBuilder.() -> Unit): UnivariateHistogram =
-        ClosedFloatingPointRange<Double> by domain.range
+        TreeHistogramBuilder(binFactory).apply(block).build()
-
-    public fun produce(builder: UnivariateHistogramBuilder.() -> Unit): UnivariateHistogram {
-        val bins: TreeMap<Double, BinCounter> = TreeMap()
-        val hBuilder = object : UnivariateHistogramBuilder {
-
-            fun get(value: Double): BinCounter? = bins.getBin(value)
-
-            fun createBin(value: Double): BinCounter {
-                val binDefinition = binFactory(value)
-                val newBin = BinCounter(binDefinition)
-                synchronized(this) { bins[binDefinition.center] = newBin }
-                return newBin
-            }
-
-            /**
-             * Thread safe put operation
-             */
-            override fun putValue(at: Double, value: Double) {
-                (get(at) ?: createBin(at)).apply {
-                    counter.add(value)
-                }
-            }
-
-            override fun putValue(point: Buffer<Double>, value: Number) {
-                put(point[0], value.toDouble())
-            }
-        }
-        hBuilder.apply(builder)
-        val resBins = TreeMap<Double, UnivariateBin>()
-        bins.forEach { (key, binCounter) ->
-            val count = binCounter.counter.value
-            resBins[key] = UnivariateBin(binCounter.domain, count, sqrt(count))
-        }
-        return TreeHistogram(resBins)
-    }
 
     override fun add(
         a: UnivariateHistogram,
@@ -89,7 +94,8 @@ public class TreeHistogramSpace(
 //        require(b.context == this) { "Histogram $b does not belong to this context" }
         val bins = TreeMap<Double, UnivariateBin>().apply {
             (a.bins.map { it.domain } union b.bins.map { it.domain }).forEach { def ->
-                put(def.center,
+                put(
+                    def.center,
                     UnivariateBin(
                         def,
                         value = (a[def.center]?.value ?: 0.0) + (b[def.center]?.value ?: 0.0),
@@ -105,7 +111,8 @@ public class TreeHistogramSpace(
     override fun scale(a: UnivariateHistogram, value: Double): UnivariateHistogram {
         val bins = TreeMap<Double, UnivariateBin>().apply {
             a.bins.forEach { bin ->
-                put(bin.domain.center,
+                put(
+                    bin.domain.center,
                     UnivariateBin(
                         bin.domain,
                         value = bin.value * value.toDouble(),
@@ -120,7 +127,7 @@ public class TreeHistogramSpace(
 
     override fun UnivariateHistogram.unaryMinus(): UnivariateHistogram = this * (-1)
 
-    override val zero: UnivariateHistogram = produce { }
+    override val zero: UnivariateHistogram by lazy { fill { } }
 
     public companion object {
         /**

kmath-histograms/src/jvmMain/kotlin/space/kscience/kmath/histogram/UnivariateHistogram.kt

@@ -13,7 +13,7 @@ import space.kscience.kmath.structures.asSequence
 
 @UnstableKMathAPI
 public val UnivariateDomain.center: Double
-    get() = (range.endInclusive - range.start) / 2
+    get() = (range.endInclusive + range.start) / 2
 
 /**
  * A univariate bin based an a range
@@ -45,7 +45,7 @@ public interface UnivariateHistogram : Histogram<Double, UnivariateBin>{
             binSize: Double,
             start: Double = 0.0,
             builder: UnivariateHistogramBuilder.() -> Unit,
-        ): UnivariateHistogram = TreeHistogramSpace.uniform(binSize, start).produce(builder)
+        ): UnivariateHistogram = TreeHistogramSpace.uniform(binSize, start).fill(builder)
 
         /**
          * Build and fill a histogram with custom borders. Returns a read-only histogram.
@@ -53,7 +53,7 @@ public interface UnivariateHistogram : Histogram<Double, UnivariateBin>{
         public fun custom(
             borders: DoubleArray,
             builder: UnivariateHistogramBuilder.() -> Unit,
-        ): UnivariateHistogram = TreeHistogramSpace.custom(borders).produce(builder)
+        ): UnivariateHistogram = TreeHistogramSpace.custom(borders).fill(builder)
 
     }
 }

kmath-histograms/src/jvmTest/kotlin/space/kscience/kmath/histogram/TreeHistogramTest.kt

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2018-2021 KMath contributors.
+ * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
+ */
+
+package space.kscience.kmath.histogram
+
+import org.junit.jupiter.api.Test
+import kotlin.random.Random
+import kotlin.test.assertTrue
+
+class TreeHistogramTest {
+
+    @Test
+    fun normalFill() {
+        val histogram = UnivariateHistogram.uniform(0.1) {
+            repeat(100_000) {
+                putValue(Random.nextDouble())
+            }
+        }
+
+        assertTrue { histogram.bins.count() > 10 }
+    }
+}