diff --git a/src/lib/index.js b/src/lib/index.js index 9c2b4b03cb1..a6086e300ce 100644 --- 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,10 +507,6 @@ 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; } @@ -505,7 +514,7 @@ lib.tagSelected = function(calcTrace, trace, ptNumber2cdIndex) { 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 index b7487eeca0b..6f3b5e36494 100644 --- 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 index ec47f64bd19..e9dc3bc0270 100644 --- 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 index b65899a88e4..8fb3c785a51 100644 --- 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 index cdef0ee4408..f084f46275a 100644 --- 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 new file mode 100644 index 00000000000..e42539a8a7d Binary files /dev/null and b/test/image/baselines/sankey_messy.png differ diff --git a/test/image/mocks/sankey_messy.json b/test/image/mocks/sankey_messy.json new file mode 100644 index 00000000000..9d7d688bb29 --- /dev/null +++ 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 index 5a666a7ac73..acc644643b3 100644 --- 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() {