From b62529dcf5761048a6cf186273bb7d71dc92bf06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Mon, 27 Mar 2017 11:59:57 -0400 Subject: [PATCH 1/5] make toBeCloseToArray and toBeCloseToArray2D handle NaN properly - as `NaN === NaN // => false` --- test/jasmine/assets/custom_matchers.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/jasmine/assets/custom_matchers.js b/test/jasmine/assets/custom_matchers.js index 5aeb2de7764..78991831cbd 100644 --- a/test/jasmine/assets/custom_matchers.js +++ b/test/jasmine/assets/custom_matchers.js @@ -143,7 +143,10 @@ function isClose(actual, expected, precision) { return Math.abs(actual - expected) < precision; } - return actual === expected; + return ( + actual === expected || + (isNaN(actual) && isNaN(expected)) + ); } function coercePosition(precision) { From 5279f999fe1c829cdf098970ad93fcb9fbc3a70a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Mon, 27 Mar 2017 12:01:01 -0400 Subject: [PATCH 2/5] set bar size in main serieslen loop - so that item with non-numeric (x,y) are excluded from the calcdata. --- src/traces/bar/calc.js | 19 +++++++++---------- test/jasmine/tests/bar_test.js | 24 +++++++++++++++++++++++- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/traces/bar/calc.js b/src/traces/bar/calc.js index 24fb3352260..bcf087bcbbc 100644 --- a/src/traces/bar/calc.js +++ b/src/traces/bar/calc.js @@ -51,15 +51,21 @@ module.exports = function calc(gd, trace) { var serieslen = Math.min(pos.length, size.length), cd = []; - // set position + // set position and size for(i = 0; i < serieslen; i++) { + var p = pos[i]; + var s = size[i]; // add bars with non-numeric sizes to calcdata // so that ensure that traces with gaps are // plotted in the correct order - if(isNumeric(pos[i])) { - cd.push({p: pos[i]}); + if(isNumeric(p)) { + if(isNumeric(s)) { + cd.push({ p: p, s: s }); + } else { + cd.push({ p: p }); + } } } @@ -84,13 +90,6 @@ module.exports = function calc(gd, trace) { } } - // set size - for(i = 0; i < cd.length; i++) { - if(isNumeric(size[i])) { - cd[i].s = size[i]; - } - } - // auto-z and autocolorscale if applicable if(hasColorscale(trace, 'marker')) { colorscaleCalc(trace, trace.marker.color, 'marker', 'c'); diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index d864a592528..b76d9912482 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -283,6 +283,28 @@ describe('Bar.calc', function() { var cd = gd.calcdata; assertPointField(cd, 'b', [[0, 1, 2], [0, 1, 0], [0, 0]]); }); + + it('should exclude items with non-numeric x/y from calcdata', function() { + var gd = mockBarPlot([{ + x: [5, NaN, 15, 20, null, 21], + y: [20, NaN, 23, 25, null, 26] + }]); + + var cd = gd.calcdata; + assertPointField(cd, 'x', [[5, 15, 20, 21]]); + assertPointField(cd, 'y', [[20, 23, 25, 26]]); + }); + + it('should not exclude items with non-numeric y from calcdata (to plots gaps correctly)', function() { + var gd = mockBarPlot([{ + x: ['a', 'b', 'c', 'd'], + y: [1, null, 'nonsense', 15] + }]); + + var cd = gd.calcdata; + assertPointField(cd, 'x', [[0, 1, 2, 3]]); + assertPointField(cd, 'y', [[1, NaN, NaN, 15]]); + }); }); describe('Bar.setPositions', function() { @@ -1285,7 +1307,7 @@ function mockBarPlot(dataWithoutTraceType, layout) { var gd = { data: dataWithTraceType, - layout: layout, + layout: layout || {}, calcdata: [] }; From 0144f06bbcb46462ca7bd1187481d38d78583e83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 28 Mar 2017 16:05:54 -0400 Subject: [PATCH 3/5] fill in non-numeric bar items in calcdata - as per AJ's recommendation - and skip over them in setPositions - use BADNUM constant to skip over non-numeric items (recall: non-numeric values are set to BADNUM in makeCalcdata) --- src/traces/bar/calc.js | 17 ++--------------- src/traces/bar/set_positions.js | 11 +++++++---- src/traces/bar/sieve.js | 3 ++- test/jasmine/tests/bar_test.js | 6 +++--- 4 files changed, 14 insertions(+), 23 deletions(-) diff --git a/src/traces/bar/calc.js b/src/traces/bar/calc.js index bcf087bcbbc..742c36f464f 100644 --- a/src/traces/bar/calc.js +++ b/src/traces/bar/calc.js @@ -49,24 +49,11 @@ module.exports = function calc(gd, trace) { // create the "calculated data" to plot var serieslen = Math.min(pos.length, size.length), - cd = []; + cd = new Array(serieslen); // set position and size for(i = 0; i < serieslen; i++) { - var p = pos[i]; - var s = size[i]; - - // add bars with non-numeric sizes to calcdata - // so that ensure that traces with gaps are - // plotted in the correct order - - if(isNumeric(p)) { - if(isNumeric(s)) { - cd.push({ p: p, s: s }); - } else { - cd.push({ p: p }); - } - } + cd[i] = { p: pos[i], s: size[i] }; } // set base diff --git a/src/traces/bar/set_positions.js b/src/traces/bar/set_positions.js index 4e4395b7490..58d2ce40a7e 100644 --- a/src/traces/bar/set_positions.js +++ b/src/traces/bar/set_positions.js @@ -10,6 +10,7 @@ 'use strict'; var isNumeric = require('fast-isnumeric'); +var BADNUM = require('../../constants/numerical').BADNUM; var Registry = require('../../registry'); var Axes = require('../../plots/cartesian/axes'); @@ -188,7 +189,7 @@ function setGroupPositionsInStackOrRelativeMode(gd, pa, sa, calcTraces) { for(var j = 0; j < calcTrace.length; j++) { var bar = calcTrace[j]; - if(!isNumeric(bar.s)) continue; + if(bar.s === BADNUM) continue; var isOutmostBar = ((bar.b + bar.s) === sieve.get(bar.p, bar.s)); if(isOutmostBar) bar._outmost = true; @@ -395,6 +396,8 @@ function setBarCenter(gd, pa, sieve) { calcBar[pLetter] = calcBar.p + ((poffsetIsArray) ? poffset[j] : poffset) + ((barwidthIsArray) ? barwidth[j] : barwidth) / 2; + + } } } @@ -502,7 +505,7 @@ function stackBars(gd, sa, sieve) { for(j = 0; j < trace.length; j++) { bar = trace[j]; - if(!isNumeric(bar.s)) continue; + if(bar.s === BADNUM) continue; // stack current bar and get previous sum var barBase = sieve.put(bar.p, bar.b + bar.s), @@ -533,7 +536,7 @@ function sieveBars(gd, sa, sieve) { for(var j = 0; j < trace.length; j++) { var bar = trace[j]; - if(isNumeric(bar.s)) sieve.put(bar.p, bar.b + bar.s); + if(bar.s !== BADNUM) sieve.put(bar.p, bar.b + bar.s); } } } @@ -569,7 +572,7 @@ function normalizeBars(gd, sa, sieve) { for(var j = 0; j < trace.length; j++) { var bar = trace[j]; - if(!isNumeric(bar.s)) continue; + if(bar.s === BADNUM) continue; var scale = Math.abs(sTop / sieve.get(bar.p, bar.s)); bar.b *= scale; diff --git a/src/traces/bar/sieve.js b/src/traces/bar/sieve.js index 55655c743c9..58f802eff31 100644 --- a/src/traces/bar/sieve.js +++ b/src/traces/bar/sieve.js @@ -11,6 +11,7 @@ module.exports = Sieve; var Lib = require('../../lib'); +var BADNUM = require('../../constants/numerical').BADNUM; /** * Helper class to sieve data from traces into bins @@ -34,7 +35,7 @@ function Sieve(traces, separateNegativeValues, dontMergeOverlappingData) { var trace = traces[i]; for(var j = 0; j < trace.length; j++) { var bar = trace[j]; - positions.push(bar.p); + if(bar.p !== BADNUM) positions.push(bar.p); } } this.positions = positions; diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index b76d9912482..abbb6ffab08 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -284,15 +284,15 @@ describe('Bar.calc', function() { assertPointField(cd, 'b', [[0, 1, 2], [0, 1, 0], [0, 0]]); }); - it('should exclude items with non-numeric x/y from calcdata', function() { + it('should not exclude items with non-numeric x/y from calcdata', function() { var gd = mockBarPlot([{ x: [5, NaN, 15, 20, null, 21], y: [20, NaN, 23, 25, null, 26] }]); var cd = gd.calcdata; - assertPointField(cd, 'x', [[5, 15, 20, 21]]); - assertPointField(cd, 'y', [[20, 23, 25, 26]]); + assertPointField(cd, 'x', [[5, NaN, 15, 20, NaN, 21]]); + assertPointField(cd, 'y', [[20, NaN, 23, 25, NaN, 26]]); }); it('should not exclude items with non-numeric y from calcdata (to plots gaps correctly)', function() { From 042cc8270e88e366721b83c96726da196143e10b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 28 Mar 2017 16:06:25 -0400 Subject: [PATCH 4/5] rm bar (now useless) 'placeholder' logic - see https://github.com/plotly/plotly.js/pull/1369 for why that was needed. --- src/traces/bar/set_positions.js | 3 +-- test/jasmine/tests/bar_test.js | 17 ----------------- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/src/traces/bar/set_positions.js b/src/traces/bar/set_positions.js index 58d2ce40a7e..cbed38a0408 100644 --- a/src/traces/bar/set_positions.js +++ b/src/traces/bar/set_positions.js @@ -38,8 +38,7 @@ module.exports = function setPositions(gd, plotinfo) { fullTrace.visible === true && Registry.traceIs(fullTrace, 'bar') && fullTrace.xaxis === xa._id && - fullTrace.yaxis === ya._id && - !calcTraces[i][0].placeholder + fullTrace.yaxis === ya._id ) { if(fullTrace.orientation === 'h') { calcTracesHorizontal.push(calcTraces[i]); diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index abbb6ffab08..5b23a80dea4 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -703,23 +703,6 @@ describe('Bar.setPositions', function() { expect(Axes.getAutoRange(ya)).toBeCloseToArray([-1.11, 1.11], undefined, '(ya.range)'); }); - it('should skip placeholder trace in position computations', function() { - var gd = mockBarPlot([{ - x: [1, 2, 3], - y: [2, 1, 2] - }, { - x: [null], - y: [null] - }]); - - expect(gd.calcdata[0][0].t.barwidth).toEqual(0.8); - - expect(gd.calcdata[1][0].x).toBe(false); - expect(gd.calcdata[1][0].y).toBe(false); - expect(gd.calcdata[1][0].placeholder).toBe(true); - expect(gd.calcdata[1][0].t.barwidth).toBeUndefined(); - }); - it('works with log axes (grouped bars)', function() { var gd = mockBarPlot([ {y: [1, 10, 1e10, -1]}, From 6f0a95d3ecea7f7ae9ed906b88cee0a96284402d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 28 Mar 2017 18:08:47 -0400 Subject: [PATCH 5/5] add non-numeric 'x' items test for bar calc.js --- test/jasmine/tests/bar_test.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index 5b23a80dea4..6bf078d4e43 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -305,6 +305,17 @@ describe('Bar.calc', function() { assertPointField(cd, 'x', [[0, 1, 2, 3]]); assertPointField(cd, 'y', [[1, NaN, NaN, 15]]); }); + + it('should not exclude items with non-numeric x from calcdata (to plots gaps correctly)', function() { + var gd = mockBarPlot([{ + x: [1, null, 'nonsense', 15], + y: [1, 2, 10, 30] + }]); + + var cd = gd.calcdata; + assertPointField(cd, 'x', [[1, NaN, NaN, 15]]); + assertPointField(cd, 'y', [[1, 2, 10, 30]]); + }); }); describe('Bar.setPositions', function() {