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 2 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
19 changes: 9 additions & 10 deletions src/traces/bar/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice. Do we need to merge in the base loop below too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? The behavior now is similar to how scatter fills in its arrayOk attributes. I can double check though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like scatter keeps a 1:1 mapping between calcdata points and the input arrays, filling in non-numerics with {x: false, y: false} - that's a good point about other arrayOK attributes, maybe it would be easier to keep that 1:1 mapping for bars too? Should be faster too, as we're not doing repeated .push, can set the size of cd at the start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer removing non-numeric items in the calc step, so that we don't have to do that during the plot step (e.g. on zoom).

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, but then how do you want to solve this for bar color arrays etc? save the original index in each cd element? cd.push({p: p, s: s, i: i}) or something? That would need to be propagated all the way through to Lib.mergeArray which seems like a bit of a disaster.

Non-numerics should be regarded as an edge case, so the overhead of managing their objects shouldn't be seen as a concern, but you're right to worry about the overhead of checking each valid point. That said, this works for scatter which can have way more points than bars, so I think as long as we make this test super simple it shouldn't be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson ok, you convinced me.

With commit 0144f06 bar calcdata has now a 1-1 correspondence with the bar (x,y) coordinates. Note that non-numeric bar items are cleaned up in bar/plot.js and makeCalcdata (called above the serieslen loop in bar/calc.js) sets non-numeric values to BADNUM.

This a pretty big change that might lead to regressions, so this PR will make be part of the next minor release v1.26.0 only.

}
}

Expand All @@ -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');
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
24 changes: 23 additions & 1 deletion 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,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]]);
});
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

});

describe('Bar.setPositions', function() {
Expand Down Expand Up @@ -1285,7 +1307,7 @@ function mockBarPlot(dataWithoutTraceType, layout) {

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

Expand Down