Streaming histogram implementation #152

marcovivero · 2018-10-08T18:04:33Z

Related issues
This is an enhancement.

Describe the proposed solution
Add additional StreamingHistogram implementation as specified here:

http://www.jmlr.org/papers/volume11/ben-haim10a/ben-haim10a.pdf

Describe alternatives you've considered
This is an alternative to the current histogram binning strategy performed in FeatureDistribution, we do a comparison on density estimation given by the two binning methods.

…into mv/distribution-simple

codecov · 2018-10-08T18:27:42Z

Codecov Report

Merging #152 into master will decrease coverage by 7.39%.
The diff coverage is 93.75%.

@@            Coverage Diff            @@
##           master     #152     +/-   ##
=========================================
- Coverage   85.74%   78.34%   -7.4%     
=========================================
  Files         302      303      +1     
  Lines        9881     9912     +31     
  Branches      334      522    +188     
=========================================
- Hits         8472     7766    -706     
- Misses       1409     2146    +737

Impacted Files	Coverage Δ
...m/salesforce/op/utils/kryo/OpKryoRegistrator.scala	`97.43% <100%> (+1.6%)`	⬆️
...sforce/op/utils/stats/RichStreamingHistogram.scala	`87.5% <87.5%> (ø)`
...ala/com/salesforce/op/testkit/InfiniteStream.scala	`0% <0%> (-100%)`	⬇️
...alesforce/op/cli/gen/templates/SimpleProject.scala	`0% <0%> (-100%)`	⬇️
...ala/com/salesforce/op/testkit/RandomIntegral.scala	`0% <0%> (-100%)`	⬇️
...scala/com/salesforce/op/testkit/RandomStream.scala	`0% <0%> (-100%)`	⬇️
...n/scala/com/salesforce/op/testkit/RandomData.scala	`0% <0%> (-100%)`	⬇️
...com/salesforce/op/testkit/ProbabilityOfEmpty.scala	`0% <0%> (-100%)`	⬇️
...om/salesforce/op/local/OpWorkflowRunnerLocal.scala	`0% <0%> (-100%)`	⬇️
.../scala/com/salesforce/op/cli/gen/ProblemKind.scala	`0% <0%> (-100%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a35108b...9c377ee. Read the comment docs.

tovbinm · 2018-10-08T18:29:47Z

utils/build.gradle

@@ -29,4 +29,7 @@ dependencies {
    compile "joda-time:joda-time:$jodaTimeVersion"
    compile "org.joda:joda-convert:$jodaConvertVersion"

+    // Breeze
+    compile 'org.scalanlp:breeze_2.11:0.13.2'


"org.scalanlp:breeze_$scalaVersion:$breezeVersion"

utils/src/main/java/com/salesforce/op/utils/stats/HistogramJavaUtils.java

tovbinm · 2018-10-08T19:47:24Z

utils/src/main/scala/com/salesforce/op/utils/stats/StreamingHistogram.scala

+        }
+      case b if b.isEmpty => 0.0
+      case b =>
+        val (p, m) = b(0) // We must have exactly one element


why not have this if/else in the case?

tovbinm · 2018-10-08T19:48:12Z

utils/src/main/scala/com/salesforce/op/utils/stats/StreamingHistogram.scala

+  }
+
+  private def updatePoints(updates: (Double, Double)*): Unit = {
+    updates.foreach { case (point, ct) =>


why mutable?!

tovbinm · 2018-10-08T19:49:03Z

utils/src/test/scala/com/salesforce/op/utils/stats/StreamingHistogramTest.scala

+    val MeanAndVariance(equiDistEmpiricalCDFMSEMean, equiDistEmpiricalCDFMSEVar, _) =
+      meanAndVariance(mses.map(_.equiDistEmpiricalCDFMSE))
+
+    println("-" * 50)


extract printing into a helper function in the spec + replace println with log.info

utils/src/test/scala/com/salesforce/op/utils/stats/StreamingHistogramTest.scala

tovbinm · 2018-10-08T19:51:19Z

utils/src/test/scala/com/salesforce/op/utils/stats/StreamingHistogramTest.scala

+    cdfTest("0.5 * Beta(5, 1) + 0.5 * Beta(1, 5)", mixture, grid, sampleSize, 400, 50)
+  }
+
+  private def cdfTest(


why there is so many private nested methods in this tests? it makes it very difficult to follow what actually the test does. can you please simplify if and use more of scalatest matchers syntax?

tovbinm · 2018-10-08T23:01:36Z

utils/src/main/java/com/salesforce/op/utils/stats/HistogramJavaUtils.java

+
+final public class HistogramJavaUtils {
+    final public static <T> TreeMap<Double, T> getTreeMap() {
+        return new TreeMap<Double, T>(getComparator());


you should be good to use TreeMap since it's already has a default serializer defined - https://github.com/EsotericSoftware/kryo/blob/kryo-parent-3.0.3/src/com/esotericsoftware/kryo/Kryo.java#L207

Just please verify it.

tovbinm · 2018-10-09T06:21:50Z

utils/src/main/java/com/salesforce/op/utils/stats/HistogramJavaUtils.java

+        return new TreeMap<Double, T>(getComparator());
+    }
+
+    final private static Comparator<Double> getComparator() {


Following upon our discussion: lets just define a concrete final comparator class

…into mv/distribution-simple

…mogrifAI-1 into mv/distribution-simple

leahmcguire

@marcovivero can you remind me why you couldnt use any existing implementations of streaming histograms?

marcovivero · 2018-10-10T22:32:30Z

@leahmcguire I was primarily trying to avoid bringing in any additional dependencies into the project.

kinfaikan · 2018-10-10T23:34:36Z

utils/src/main/scala/com/salesforce/op/utils/stats/StreamingHistogram.scala

+  private[stats] def sum(bins: Array[Bin], x: Double): Double =
+    bins.sliding(2).foldLeft((0.0, EmptyBin, false)) { case ((s, _, done), arr) =>
+      arr match {
+        case Array((p, m)) =>


Do we need this case?

kinfaikan · 2018-10-10T23:34:41Z

utils/src/main/scala/com/salesforce/op/utils/stats/StreamingHistogram.scala

+          (newS, EmptyBin, true)
+
+        case Array((p1, m1), (p2, m2)) =>
+          if (x >= p1 && x < p2 && !done) {


Slightly simplified?

if (done || x < p1) { (s, (p2, m2), true) } else if (x >= p1 && x < p2) { val mx = m1 + ((m2 - m1) / (p2 - p1)) * (x - p1) val newS = s + (m1 / 2) + ((m1 + mx) / 2) * ((x - p1) / (p2 - p1)) (newS, (p2, m2), true) } else { (s + m1, (p2, m2), false) }

kinfaikan · 2018-10-10T23:57:42Z

utils/src/main/scala/com/salesforce/op/utils/stats/StreamingHistogram.scala

+  final def empiricalCDF(x: Double): Double = StreamingHistogram.empiricalCDF(getBins, x)
+
+  private[this] def mergePoints(): Unit = if (points.size > math.max(maxBins, 2)) {
+    val (q1, q2) = points.descendingKeySet.descendingIterator.asScala.sliding(2).map(_ match {


Counts are not considered in points merging?

leahmcguire · 2018-10-11T16:19:51Z

small dependancies for tested and maintained code are better than reinventing the wheel.

marcovivero · 2018-10-11T20:05:36Z

@leahmcguire: per @tovbinm's suggestion I grabbed out the code from Cassandra implementation and modified a bit for this use case.

tovbinm · 2018-10-12T05:02:05Z

utils/src/main/scala/com/salesforce/op/utils/kryo/OpKryoRegistrator.scala

@@ -67,6 +70,15 @@ class OpKryoRegistrator extends KryoRegistrator {
      classOf[GenericData.Array[_]],
      new GenericJavaCollectionSerializer(classOf[java.util.ArrayList[_]])
    )
+
+    // Streaming histogram registration
+    doClassRegistration(kryo)(


hmm, we should not be doing this for TreeMap and mutable.WrappedArray, as those should be already registered with Kryo.

This flag causes a failure if these aren't explicitly registered here:

https://github.com/salesforce/TransmogrifAI/pull/152/files#diff-36f3a0a7cf47675bc10aac982dab7fecR51

This is weird and to my understanding should not be the case. Let's find time to look at together.

tovbinm · 2018-10-12T05:03:03Z

utils/src/main/scala/com/salesforce/op/utils/stats/HistogramUtils.scala

+
+import scala.collection.JavaConverters._
+
+object HistogramUtils {


docs please: for object and methods.

leahmcguire · 2018-10-12T16:37:54Z

utils/src/main/java/com/salesforce/op/utils/stats/StreamingHistogram.java

+ * Yael Ben-Haim and Elad Tom-Tov, "A Streaming Parallel Decision Tree Algorithm" (2010)
+ * http://jmlr.csail.mit.edu/papers/volume11/ben-haim10a/ben-haim10a.pdf
+ *
+ * This implementation is from Apache Cassandra roject, release 3.11.


leahmcguire · 2018-10-12T16:43:55Z

utils/src/test/scala/com/salesforce/op/utils/stats/StreamingHistogramTest.scala

+    println("Equidistant histogram density MSE mean and variance: " +
+      s"${result.equiDistDensityMSE.mean}, ${result.equiDistDensityMSE.variance}")
+    println("Equidistant histogram cdf MSE mean and variance: " +
+      s"${result.equiDistCdfMSE.mean}, ${result.equiDistCdfMSE.variance}")


please add some asserts and change these prints to logs that can be turned on or off

tovbinm · 2018-10-12T18:49:24Z

utils/src/main/java/com/salesforce/op/utils/stats/StreamingHistogram.java

+ */
+package com.salesforce.op.utils.stats;
+
+import java.io.IOException;


no need to import java.io.IOException;

tovbinm · 2018-10-12T18:55:11Z

utils/src/test/scala/com/salesforce/op/utils/stats/StreamingHistogramTest.scala

+    val gaussian2 = Gaussian(5, 5)
+    val mixture = MixtureDistribution(gaussian1, gaussian2, 0.8, sampleSize)
+
+    cdfTest("Gaussian(0, 1)", gaussian1, histogramSampleSize, 100, mcSampleSize, 50)


let's not run every test 50 times. that is very slow. let's add this as val and maybe do just 3.

tovbinm · 2018-10-12T20:00:40Z

utils/src/main/scala/com/salesforce/op/utils/kryo/OpKryoRegistrator.scala

+      classOf[StreamingHistogram],
+      classOf[StreamingHistogramBuilder],
+      classOf[StreamingHistogramComparator],
+      classOf[TreeMap[_, _]],


do this instead

import scala.collection.mutable.{WrappedArray => MWrappedArray} // Streaming histogram registration kryo.register(classOf[StreamingHistogram]) kryo.register(classOf[StreamingHistogramBuilder]) kryo.register(classOf[StreamingHistogramComparator]) kryo.register(classOf[TreeMap[_, _]], new TreeMapSerializer()) // Mutable wrapped arrays OpKryoClasses.WrappedArrays.foreach(kryo.register)

where

private[op] case object OpKryoClasses { val WrappedArrays: Seq[Class[_]] = Seq( MWrappedArray.make(Array[Boolean]()).getClass, MWrappedArray.make(Array[Byte]()).getClass, MWrappedArray.make(Array[Char]()).getClass, MWrappedArray.make(Array[Double]()).getClass, MWrappedArray.make(Array[Float]()).getClass, MWrappedArray.make(Array[Int]()).getClass, MWrappedArray.make(Array[Long]()).getClass, MWrappedArray.make(Array[Short]()).getClass, MWrappedArray.make(Array[String]()).getClass ) }

…mogrifAI-1 into mv/distribution-simple

tovbinm · 2018-10-16T16:48:37Z

utils/src/test/scala/com/salesforce/op/utils/stats/StreamingHistogramTest.scala

+    val gaussian = Gaussian(0, 1)(RandBasis.mt0)
+    val distributionName = "Gaussian(0, 1)"
+
+    val result75 = distributionTestResult(distributionName, gaussian, histogramSampleSize, 75, mcSampleSize, 5)


please add val numResults = 5 in the head of the test.

tovbinm

one minor comment about numResults, otherwise lgtm!

tovbinm · 2018-10-16T16:50:15Z

utils/src/main/java/com/salesforce/op/utils/stats/StreamingHistogram.java

@@ -0,0 +1,299 @@
+/*


I think you need to add // scalastyle:off header.matches line here.

and also include it to the exclusion list - https://github.com/salesforce/TransmogrifAI/blob/master/build.gradle#L205

This class won't go through scalastyle since in src/main/java so I don't think that's needed.

…mogrifAI-1 into mv/distribution-simple

* Rebase adaptive histogram implementation * Add test * Outlier test * Histogram final defs * Add distribution tests * Update distribution tests * Use cassandra implementation * Use correct Kryo serializers * Clean up tests + address comments * Address comments

marcovivero added 6 commits September 13, 2018 12:42

Rebase adaptive histogram implementation

dcb3726

Add test

aacc7ed

Outlier test

ac00735

Histogram final defs

923b891

Merge branch 'master' of https://github.com/salesforce/TransmogrifAI …

efee21c

…into mv/distribution-simple

Add distribution tests

547cee3

marcovivero requested review from leahmcguire and tovbinm as code owners October 8, 2018 18:04

salesforce-cla bot added the cla:signed label Oct 8, 2018

marcovivero requested a review from Jauntbox October 8, 2018 18:04

marcovivero changed the title ~~Mv/distribution simple~~ Streaming histogram implementation Oct 8, 2018

Merge branch 'master' into mv/distribution-simple

3e38f12

tovbinm reviewed Oct 8, 2018

View reviewed changes

tovbinm reviewed Oct 9, 2018

View reviewed changes

marcovivero added 2 commits October 8, 2018 23:45

Merge branch 'master' of https://github.com/salesforce/TransmogrifAI …

d658b5a

…into mv/distribution-simple

Merge branch 'mv/distribution-simple' of github.com:marcovivero/Trans…

42ff372

…mogrifAI-1 into mv/distribution-simple

marcovivero requested a review from kinfaikan October 9, 2018 17:58

Update distribution tests

cd8e67f

leahmcguire reviewed Oct 10, 2018

View reviewed changes

kinfaikan reviewed Oct 10, 2018

View reviewed changes

tovbinm reviewed Oct 12, 2018

View reviewed changes

Use cassandra implementation

f5268ba

leahmcguire reviewed Oct 12, 2018

View reviewed changes

tovbinm reviewed Oct 12, 2018

View reviewed changes

marcovivero and others added 4 commits October 12, 2018 15:47

Use correct Kryo serializers

8011e94

Merge branch 'master' into mv/distribution-simple

5c41734

Clean up tests + address comments

4d96ce4

Merge branch 'mv/distribution-simple' of github.com:marcovivero/Trans…

c6be4c5

…mogrifAI-1 into mv/distribution-simple

tovbinm reviewed Oct 16, 2018

View reviewed changes

tovbinm approved these changes Oct 16, 2018

View reviewed changes

tovbinm reviewed Oct 16, 2018

View reviewed changes

tovbinm and others added 3 commits October 16, 2018 10:23

Merge branch 'master' into mv/distribution-simple

4346e87

Address comments

8755d42

Merge branch 'mv/distribution-simple' of github.com:marcovivero/Trans…

9c377ee

…mogrifAI-1 into mv/distribution-simple

marcovivero merged commit 8264265 into salesforce:master Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streaming histogram implementation #152

Streaming histogram implementation #152

marcovivero commented Oct 8, 2018 •

edited

Loading

codecov bot commented Oct 8, 2018 •

edited

Loading

tovbinm Oct 8, 2018

tovbinm Oct 8, 2018

tovbinm Oct 8, 2018

tovbinm Oct 8, 2018

tovbinm Oct 8, 2018

tovbinm Oct 8, 2018

tovbinm Oct 9, 2018

tovbinm Oct 9, 2018

leahmcguire left a comment

marcovivero commented Oct 10, 2018

kinfaikan Oct 10, 2018

kinfaikan Oct 10, 2018

kinfaikan Oct 10, 2018

leahmcguire commented Oct 11, 2018

marcovivero commented Oct 11, 2018

tovbinm Oct 12, 2018

marcovivero Oct 12, 2018

tovbinm Oct 12, 2018

tovbinm Oct 12, 2018

leahmcguire Oct 12, 2018

leahmcguire Oct 12, 2018

tovbinm Oct 12, 2018

tovbinm Oct 12, 2018

tovbinm Oct 12, 2018 •

edited

Loading

tovbinm Oct 16, 2018

tovbinm left a comment

tovbinm Oct 16, 2018

tovbinm Oct 16, 2018

marcovivero Oct 16, 2018


		import scala.collection.JavaConverters._

		object HistogramUtils {

Streaming histogram implementation #152

Streaming histogram implementation #152

Conversation

marcovivero commented Oct 8, 2018 • edited Loading

codecov bot commented Oct 8, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leahmcguire left a comment

Choose a reason for hiding this comment

marcovivero commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leahmcguire commented Oct 11, 2018

marcovivero commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tovbinm Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tovbinm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcovivero commented Oct 8, 2018 •

edited

Loading

codecov bot commented Oct 8, 2018 •

edited

Loading

tovbinm Oct 12, 2018 •

edited

Loading