Skip to content

Bar with non-numeric (x,y) items bug fix #1519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 30, 2017
Merged

Bar with non-numeric (x,y) items bug fix #1519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b62529d
make toBeCloseToArray and toBeCloseToArray2D handle NaN properly
etpinard Mar 27, 2017
5279f99
set bar size in main serieslen loop
etpinard Mar 27, 2017
0144f06
fill in non-numeric bar items in calcdata
etpinard Mar 28, 2017
042cc82
rm bar (now useless) 'placeholder' logic
etpinard Mar 28, 2017
6f0a95d
add non-numeric 'x' items test for bar calc.js
etpinard Mar 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 3 additions & 17 deletions src/traces/bar/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +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
// set position and size
for(i = 0; i < serieslen; 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]});
}
cd[i] = { p: pos[i], s: size[i] };
}

// set base
Expand All @@ -84,13 +77,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');
Expand Down
14 changes: 8 additions & 6 deletions src/traces/bar/set_positions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -37,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]);
Expand Down Expand Up @@ -188,7 +188,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;
Expand Down Expand Up @@ -395,6 +395,8 @@ function setBarCenter(gd, pa, sieve) {
calcBar[pLetter] = calcBar.p +
((poffsetIsArray) ? poffset[j] : poffset) +
((barwidthIsArray) ? barwidth[j] : barwidth) / 2;


}
}
}
Expand Down Expand Up @@ -502,7 +504,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),
Expand Down Expand Up @@ -533,7 +535,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);
}
}
}
Expand Down Expand Up @@ -569,7 +571,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;
Expand Down
3 changes: 2 additions & 1 deletion src/traces/bar/sieve.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
5 changes: 4 additions & 1 deletion test/jasmine/assets/custom_matchers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
52 changes: 34 additions & 18 deletions test/jasmine/tests/bar_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,39 @@ describe('Bar.calc', function() {
var cd = gd.calcdata;
assertPointField(cd, 'b', [[0, 1, 2], [0, 1, 0], [0, 0]]);
});

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, 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() {
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]]);
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have a test with a bad x but good y for the same point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Added in 6f0a95d


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() {
Expand Down Expand Up @@ -681,23 +714,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]},
Expand Down Expand Up @@ -1285,7 +1301,7 @@ function mockBarPlot(dataWithoutTraceType, layout) {

var gd = {
data: dataWithTraceType,
layout: layout,
layout: layout || {},
calcdata: []
};

Expand Down