Fix NaN values confusing closest-point search

The current algorithm was not handling NaN values right, the "dist >= max"
check is false for dist=NaN and led to the "this point is closer" branch
being taken. Fix this by reversing the if logic, and add a regression
test for it. (I confirmed that the test fails for the original code,
and passes after the change.)

diff --git a/auto_tests/tests/callback.js b/auto_tests/tests/callback.js
index fe7f7fd..a26b7e7 100644 (file)
--- a/auto_tests/tests/callback.js
+++ b/auto_tests/tests/callback.js
@@ -244,6 +244,7 @@ CallbackTestCase.prototype.testClosestPointCallbackCss1 = function() {
     "div.dygraph-legend > span.highlight { border: 1px solid grey; }\n";
   this.styleSheet.innerHTML = css;
   runClosestTest(false, 2, 4);
+  this.styleSheet.innerHTML = '';
 }
 
 /**
@@ -254,5 +255,55 @@ CallbackTestCase.prototype.testClosestPointCallbackCss2 = function() {
     "div.dygraph-legend > span.highlight { display: inline; }\n";
   this.styleSheet.innerHTML = css;
   runClosestTest(false, 10, 15);
+  this.styleSheet.innerHTML = '';
   // TODO(klausw): verify that the highlighted line is drawn on top?
 }
+
+/**
+ * This tests that closest point searches work for data containing NaNs.
+ *
+ * It's intended to catch a regression where a NaN Y value confuses the
+ * closest-point algorithm, treating it as closer as any previous point.
+ */
+CallbackTestCase.prototype.testNaNData = function() {
+  var dataNaN = [
+    [10, -1, 1, 2],
+    [11, 0, 3, 1],
+    [12, 1, 4, NaN],
+    [13, 0, 2, 3],
+    [14, -1, 1, 4]];
+
+  var h_row;
+  var h_pts;
+
+  var highlightCallback  =  function(e, x, pts, row) {
+    h_row = row;
+    h_pts = pts;
+  };
+
+  var graph = document.getElementById("graph");
+  var g = new Dygraph(graph, dataNaN,
+      {
+        width: 600,
+        height: 400,
+        labels: ['x', 'a', 'b', 'c'],
+        visibility: [false, true, true],
+        highlightCallback: highlightCallback
+      });
+
+  DygraphOps.dispatchMouseMove(g, 10.1, 0.9);
+  //check correct row is returned
+  assertEquals(0, h_row);
+
+  // Explicitly test closest point algorithms
+  var dom = g.toDomCoords(10.1, 0.9);
+  assertEquals(0, g.findClosestRow(dom[0]));
+
+  var res = g.findClosestPoint(dom[0], dom[1]);
+  assertEquals(0, res.row);
+  assertEquals('b', res.seriesName);
+
+  res = g.findStackedPoint(dom[0], dom[1]);
+  assertEquals(0, res.row);
+  assertEquals('c', res.seriesName);
+};

diff --git a/dygraph.js b/dygraph.js
index 3502c31..bc830b2 100644 (file)
--- a/dygraph.js
+++ b/dygraph.js
@@ -1517,9 +1517,10 @@ Dygraph.prototype.findClosestRow = function(domX) {
     var point = points[i];
     if (point === null) continue;
     var dist = Math.abs(point.canvasx - domX);
-    if (minDistX !== null && dist >= minDistX) continue;
-    minDistX = dist;
-    idx = i;
+    if (minDistX === null || dist < minDistX) {
+      minDistX = dist;
+      idx = i;
+    }
   }
   return this.idxToRow_(idx);
 };
@@ -1550,11 +1551,12 @@ Dygraph.prototype.findClosestPoint = function(domX, domY) {
       dx = point.canvasx - domX;
       dy = point.canvasy - domY;
       dist = dx * dx + dy * dy;
-      if (minDist !== null && dist >= minDist) continue;
-      minDist = dist;
-      closestPoint = point;
-      closestSeries = setIdx;
-      idx = i;
+      if (minDist === null || dist < minDist) {
+        minDist = dist;
+        closestPoint = point;
+        closestSeries = setIdx;
+        idx = i;
+      }
     }
   }
   var name = this.layout_.setNames[closestSeries];
@@ -1605,9 +1607,10 @@ Dygraph.prototype.findStackedPoint = function(domX, domY) {
       }
     }
     // Stop if the point (domX, py) is above this series' upper edge
-    if (setIdx > 0 && py >= domY) break;
-    closestPoint = p1;
-    closestSeries = setIdx;
+    if (setIdx == 0 || py < domY) {
+      closestPoint = p1;
+      closestSeries = setIdx;
+    }
   }
   var name = this.layout_.setNames[closestSeries];
   return {