From 906d65c06f37ccf74f6c13df73c83293d1be7290 Mon Sep 17 00:00:00 2001
From: Bruce Martin <bruce@chanzuckerberg.com>
Date: Mon, 5 Aug 2019 10:40:56 -0700
Subject: [PATCH] fix binning error in continuous data histogram (#869)

---
 .../util/dataframe/histogram.test.js          | 24 +++++++++++++++++++
 client/src/util/dataframe/histogram.js        | 10 ++++----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/client/__tests__/util/dataframe/histogram.test.js b/client/__tests__/util/dataframe/histogram.test.js
index 45dc152c..1549d27d 100644
--- a/client/__tests__/util/dataframe/histogram.test.js
+++ b/client/__tests__/util/dataframe/histogram.test.js
@@ -66,4 +66,28 @@ describe("Dataframe column histogram", () => {
     // memoized?
     expect(df.col("value").histogram(3, [0, 2])).toMatchObject(h1);
   });
+
+  test("continuous thesholds correct", () => {
+    const vals = [0, 1, 9, 10, 11, 20, 99, 100];
+    const df = new Dataframe.Dataframe(
+      [8, 2],
+      [new Int32Array(vals), new Float32Array(vals)]
+    );
+
+    expect(df.col(0).histogram(5, [0, 100])).toEqual([5, 1, 0, 0, 2]);
+    expect(df.col(1).histogram(5, [0, 100])).toEqual([5, 1, 0, 0, 2]);
+    expect(df.col(0).histogram(2, [0, 10])).toEqual([2, 2]);
+    expect(df.col(0).histogram(10, [0, 100])).toEqual([
+      3,
+      2,
+      1,
+      0,
+      0,
+      0,
+      0,
+      0,
+      0,
+      2
+    ]);
+  });
 });
diff --git a/client/src/util/dataframe/histogram.js b/client/src/util/dataframe/histogram.js
index 09f0d17d..48d6a19e 100644
--- a/client/src/util/dataframe/histogram.js
+++ b/client/src/util/dataframe/histogram.js
@@ -8,13 +8,13 @@ function _histogramContinuous(column, bins, min, max) {
   if (!column) {
     return valBins;
   }
-  const binWidth = (max - min) / (bins - 1);
+  const binWidth = (max - min) / bins;
   const colArray = column.asArray();
   for (let r = 0, len = colArray.length; r < len; r += 1) {
     const val = colArray[r];
     if (val <= max && val >= min) {
       // ensure test excludes NaN values
-      const valBin = (val - min) / binWidth;
+      const valBin = Math.min(Math.floor((val - min) / binWidth), bins - 1);
       valBins[valBin] += 1;
     }
   }
@@ -26,7 +26,7 @@ function _histogramContinuousBy(column, bins, min, max, by) {
   if (!column || !by) {
     return byMap;
   }
-  const binWidth = (max - min) / (bins - 1);
+  const binWidth = (max - min) / bins;
   const byArray = by.asArray();
   const colArray = column.asArray();
   for (let r = 0, len = colArray.length; r < len; r += 1) {
@@ -39,8 +39,8 @@ function _histogramContinuousBy(column, bins, min, max, by) {
     const val = colArray[r];
     if (val <= max && val >= min) {
       // ensure test excludes NaN values
-      const valBin = (val - min) / binWidth;
-      valBins[Math.floor(valBin)] += 1;
+      const valBin = Math.min(Math.floor((val - min) / binWidth), bins - 1);
+      valBins[valBin] += 1;
     }
   }
   return byMap;