Merge pull request #2629 from plotly/sankey-overlap-fix

alexcjohnson · web-flow · commit 9d6144304308 · 2018-05-11T17:34:09.000-04:00
More robust Sankey
diff --git a/src/lib/index.js b/src/lib/index.js
@@ -134,6 +134,19 @@ lib.ensureNumber = function num(v) {
     return isNumeric(v) ? Number(v) : BADNUM;
 };
 
+/**
+ * Is v a valid array index? Accepts numeric strings as well as numbers.
+ *
+ * @param {any} v: the value to test
+ * @param {Optional[integer]} len: the array length we are indexing
+ *
+ * @return {bool}: v is a valid array index
+ */
+lib.isIndex = function(v, len) {
+    if(len !== undefined && v >= len) return false;
+    return isNumeric(v) && (v >= 0) && (v % 1 === 0);
+};
+
 lib.noop = require('./noop');
 lib.identity = require('./identity');
 
@@ -494,18 +507,14 @@ lib.tagSelected = function(calcTrace, trace, ptNumber2cdIndex) {
         }
     }
 
-    function isPtIndexValid(v) {
-        return isNumeric(v) && v >= 0 && v % 1 === 0;
-    }
-
     function isCdIndexValid(v) {
         return v !== undefined && v < calcTrace.length;
     }
 
     for(var i = 0; i < selectedpoints.length; i++) {
         var ptIndex = selectedpoints[i];
 
-        if(isPtIndexValid(ptIndex)) {
+        if(lib.isIndex(ptIndex)) {
             var ptNumber = ptIndex2ptNumber ? ptIndex2ptNumber[ptIndex] : ptIndex;
             var cdIndex = ptNumber2cdIndex ? ptNumber2cdIndex[ptNumber] : ptNumber;
 
diff --git a/src/plot_api/plot_schema.js b/src/plot_api/plot_schema.js
@@ -437,6 +437,8 @@ function recurseIntoValObject(valObject, parts, i) {
     return valObject;
 }
 
+// note: this is different from Lib.isIndex, this one doesn't accept numeric
+// strings, only actual numbers.
 function isIndex(val) {
     return val === Math.round(val) && val >= 0;
 }
diff --git a/src/traces/sankey/calc.js b/src/traces/sankey/calc.js
@@ -14,13 +14,16 @@ var wrap = require('../../lib/gup').wrap;
 
 function circularityPresent(nodeList, sources, targets) {
 
-    var nodes = nodeList.map(function() {return [];});
+    var nodeLen = nodeList.length;
+    var nodes = Lib.init2dArray(nodeLen, 0);
 
     for(var i = 0; i < Math.min(sources.length, targets.length); i++) {
-        if(sources[i] === targets[i]) {
-            return true; // self-link which is also a scc of one
+        if(Lib.isIndex(sources[i], nodeLen) && Lib.isIndex(targets[i], nodeLen)) {
+            if(sources[i] === targets[i]) {
+                return true; // self-link which is also a scc of one
+            }
+            nodes[sources[i]].push(targets[i]);
         }
-        nodes[sources[i]].push(targets[i]);
     }
 
     var scc = tarjan(nodes);
diff --git a/src/traces/sankey/defaults.js b/src/traces/sankey/defaults.js
@@ -55,15 +55,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
 
     Lib.coerceFont(coerce, 'textfont', Lib.extendFlat({}, layout.font));
 
-    var missing = function(n, i) {
-        return traceOut.link.source.indexOf(i) === -1 &&
-            traceOut.link.target.indexOf(i) === -1;
-    };
-
-    if(traceOut.node.label.some(missing)) {
-        Lib.warn('Some of the nodes are neither sources nor targets, they will not be displayed.');
-    }
-
     // disable 1D transforms - arrays here are 1D but their lengths/meanings
     // don't match, between nodes and links
     traceOut._length = null;
diff --git a/src/traces/sankey/render.js b/src/traces/sankey/render.js
@@ -16,9 +16,12 @@ var Drawing = require('../../components/drawing');
 var d3sankey = require('@plotly/d3-sankey').sankey;
 var d3Force = require('d3-force');
 var Lib = require('../../lib');
-var keyFun = require('../../lib/gup').keyFun;
-var repeat = require('../../lib/gup').repeat;
-var unwrap = require('../../lib/gup').unwrap;
+var isArrayOrTypedArray = Lib.isArrayOrTypedArray;
+var isIndex = Lib.isIndex;
+var gup = require('../../lib/gup');
+var keyFun = gup.keyFun;
+var repeat = gup.repeat;
+var unwrap = gup.unwrap;
 
 // basic data utilities
 
@@ -63,44 +66,79 @@ function switchToSankeyFormat(nodes) {
 
 // view models
 
-function sankeyModel(layout, d, i) {
-    var trace = unwrap(d).trace,
-        domain = trace.domain,
-        nodeSpec = trace.node,
-        linkSpec = trace.link,
-        arrangement = trace.arrangement,
-        horizontal = trace.orientation === 'h',
-        nodePad = trace.node.pad,
-        nodeThickness = trace.node.thickness,
-        nodeLineColor = trace.node.line.color,
-        nodeLineWidth = trace.node.line.width,
-        linkLineColor = trace.link.line.color,
-        linkLineWidth = trace.link.line.width,
-        valueFormat = trace.valueformat,
-        valueSuffix = trace.valuesuffix,
-        textFont = trace.textfont;
-
-    var width = layout.width * (domain.x[1] - domain.x[0]),
-        height = layout.height * (domain.y[1] - domain.y[0]);
-
-    var nodes = nodeSpec.label.map(function(l, i) {
-        return {
-            pointNumber: i,
-            label: l,
-            color: Lib.isArrayOrTypedArray(nodeSpec.color) ? nodeSpec.color[i] : nodeSpec.color
-        };
-    });
+function sankeyModel(layout, d, traceIndex) {
+    var trace = unwrap(d).trace;
+    var domain = trace.domain;
+    var nodeSpec = trace.node;
+    var linkSpec = trace.link;
+    var arrangement = trace.arrangement;
+    var horizontal = trace.orientation === 'h';
+    var nodePad = trace.node.pad;
+    var nodeThickness = trace.node.thickness;
+    var nodeLineColor = trace.node.line.color;
+    var nodeLineWidth = trace.node.line.width;
+    var linkLineColor = trace.link.line.color;
+    var linkLineWidth = trace.link.line.width;
+    var valueFormat = trace.valueformat;
+    var valueSuffix = trace.valuesuffix;
+    var textFont = trace.textfont;
+
+    var width = layout.width * (domain.x[1] - domain.x[0]);
+    var height = layout.height * (domain.y[1] - domain.y[0]);
+
+    var links = [];
+    var hasLinkColorArray = isArrayOrTypedArray(linkSpec.color);
+    var linkedNodes = {};
+
+    var nodeCount = nodeSpec.label.length;
+    var i;
+    for(i = 0; i < linkSpec.value.length; i++) {
+        var val = linkSpec.value[i];
+        // remove negative values, but keep zeros with special treatment
+        var source = linkSpec.source[i];
+        var target = linkSpec.target[i];
+        if(!(val > 0 && isIndex(source, nodeCount) && isIndex(target, nodeCount))) {
+            continue;
+        }
+
+        source = +source;
+        target = +target;
+        linkedNodes[source] = linkedNodes[target] = true;
 
-    var links = linkSpec.value.map(function(d, i) {
-        return {
+        links.push({
             pointNumber: i,
             label: linkSpec.label[i],
-            color: Lib.isArrayOrTypedArray(linkSpec.color) ? linkSpec.color[i] : linkSpec.color,
-            source: linkSpec.source[i],
-            target: linkSpec.target[i],
-            value: d
-        };
-    });
+            color: hasLinkColorArray ? linkSpec.color[i] : linkSpec.color,
+            source: source,
+            target: target,
+            value: +val
+        });
+    }
+
+    var hasNodeColorArray = isArrayOrTypedArray(nodeSpec.color);
+    var nodes = [];
+    var removedNodes = false;
+    var nodeIndices = {};
+    for(i = 0; i < nodeCount; i++) {
+        if(linkedNodes[i]) {
+            var l = nodeSpec.label[i];
+            nodeIndices[i] = nodes.length;
+            nodes.push({
+                pointNumber: i,
+                label: l,
+                color: hasNodeColorArray ? nodeSpec.color[i] : nodeSpec.color
+            });
+        }
+        else removedNodes = true;
+    }
+
+    // need to re-index links now, since we didn't put all the nodes in
+    if(removedNodes) {
+        for(i = 0; i < links.length; i++) {
+            links[i].source = nodeIndices[links[i].source];
+            links[i].target = nodeIndices[links[i].target];
+        }
+    }
 
     var sankey = d3sankey()
         .size(horizontal ? [width, height] : [height, width])
@@ -120,7 +158,7 @@ function sankeyModel(layout, d, i) {
     switchToForceFormat(nodes);
 
     return {
-        key: i,
+        key: traceIndex,
         trace: trace,
         guid: Math.floor(1e12 * (1 + Math.random())),
         horizontal: horizontal,
@@ -430,7 +468,6 @@ module.exports = function(svg, styledData, layout, callbacks) {
         .style('left', 0)
         .style('shape-rendering', 'geometricPrecision')
         .style('pointer-events', 'auto')
-        .style('box-sizing', 'content-box')
         .attr('transform', sankeyTransform);
 
     sankey.transition()
diff --git a/test/image/baselines/sankey_messy.png b/test/image/baselines/sankey_messy.png
diff --git a/test/image/mocks/sankey_messy.json b/test/image/mocks/sankey_messy.json
@@ -0,0 +1,43 @@
+{
+    "data": [
+        {
+            "type": "sankey",
+            "node": {
+                "pad": 5,
+                "label": [
+                    "a", "b", "c", "d", "e", "f", "g", "h", "i", "j",
+                    "10", 11, "12", 13, 14, 15, 16, 17, 18, 19,
+                    20, "21", 22, "23", 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42,
+                    "k", "l", "m", "n", "o", "p", "q", "r", "s", "t"
+                ]
+            },
+            "link": {
+                "source": [
+                    "", "x", 20, 21, 20, 21,
+                    20, 21, 22, 23, 24, 25, 20, 26, 22, 23, 27, 28,
+                    22, 29, 26, 28, 23, 30, 31, 32, 29, 26, 33, 34,
+                    24, 35, 36, 37, 38, 26, 22, 39, 36, 27, 26, 23,
+                    "40", "22", "41", "35", 42, 27, 40, 20, 28
+                ],
+                "target": [
+                    10, 10, "", "q", 10, 10,
+                    10, 10, 10, 10, 10, 11, 11, 11, 11, 11, 12, 12,
+                    12, 13, 13, 13, 13, 14, 14, 14, 14, 14, 15, 15,
+                    15, 15, 16, 16, 16, 16, 16, 16, 17, 17, 17, 17,
+                    18, 18, "18", "18", 19, 19, "19", "19", 19
+                ],
+                "value": [
+                    100, 100, 100, 100, -15, "nope",
+                    48, 40, 44, 4, 14, 0, 40, 25, 50, 137, 149, 100,
+                    100, 10, 200, 149, 237, 11, 130, 40, 10, 200, 50, 100,
+                    13, 100, 20, 100, 20, 100, 100, 7, 150, 40, 50, 11,
+                    "50", 80, "2", 40, "50", 50, "90", 125, 50
+                ]
+            }
+        }],
+    "layout": {
+        "title": "Sankey with messy data",
+        "width": 600,
+        "height": 800
+    }
+}
diff --git a/test/jasmine/tests/sankey_test.js b/test/jasmine/tests/sankey_test.js
@@ -58,30 +58,31 @@ describe('sankey tests', function() {
         });
     });
 
-    describe('log warning if issue is encountered with graph structure',
-        function() {
-
-            it('some nodes are not linked', function() {
-
-                var warnings = [];
-                spyOn(Lib, 'warn').and.callFake(function(msg) {
-                    warnings.push(msg);
-                });
-
-                _supply({
-                    node: {
-                        label: ['a', 'b', 'c']
-                    },
-                    link: {
-                        value: [1],
-                        source: [0],
-                        target: [1]
-                    }
-                });
+    describe('No warnings for missing nodes', function() {
+        // we used to warn when some nodes were not used in the links
+        // not doing that anymore, it's not really consistent with
+        // the rest of our data processing.
+        it('some nodes are not linked', function() {
+
+            var warnings = [];
+            spyOn(Lib, 'warn').and.callFake(function(msg) {
+                warnings.push(msg);
+            });
 
-                expect(warnings.length).toEqual(1);
+            _supply({
+                node: {
+                    label: ['a', 'b', 'c']
+                },
+                link: {
+                    value: [1],
+                    source: [0],
+                    target: [1]
+                }
             });
+
+            expect(warnings.length).toEqual(0);
         });
+    });
 
     describe('sankey global defaults', function() {
 

Original file line number	Diff line number	Diff line change
`@@ -437,6 +437,8 @@ function recurseIntoValObject(valObject, parts, i) {`
`437`	`437`	`return valObject;`
`438`	`438`	`}`
`439`	`439`
	`440`	`+// note: this is different from Lib.isIndex, this one doesn't accept numeric`
	`441`	`+// strings, only actual numbers.`
`440`	`442`	`function isIndex(val) {`
`441`	`443`	`return val === Math.round(val) && val >= 0;`
`442`	`444`	`}`