Skip to content

Commit aa70e4c

Browse files
monferamonfera
committed
PR feedback: don't split if groups is unsupplied, non-array or of zero length
1 parent 79dcbc3 commit aa70e4c

File tree

2 files changed

+96
-87
lines changed

2 files changed

+96
-87
lines changed

‎src/transforms/groupby.js‎

Lines changed: 36 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,38 @@ exports.transform = function(data, state) {
9898
return newData;
9999
};
100100

101+
function getDeepProp(thing, propArray) {
102+
var result = thing;
103+
var i;
104+
for(i = 0; i < propArray.length; i++) {
105+
result = result[propArray[i]];
106+
if(result === void(0)) {
107+
return result;
108+
}
109+
}
110+
return result;
111+
}
112+
113+
function setDeepProp(thing, propArray, value) {
114+
var current = thing;
115+
var i;
116+
for(i = 0; i < propArray.length - 1; i++) {
117+
if(current[propArray[i]] === void(0)) {
118+
current[propArray[i]] = {};
119+
}
120+
current = current[propArray[i]];
121+
}
122+
current[propArray[propArray.length - 1]] = value;
123+
}
124+
125+
function initializeArray(newTrace, a) {
126+
setDeepProp(newTrace, a, []);
127+
}
128+
129+
function pasteArray(newTrace, trace, j, a) {
130+
getDeepProp(newTrace, a).push(getDeepProp(trace, a)[j]);
131+
}
132+
101133
function transformOne(trace, state, attributeSet) {
102134

103135
var opts = state.transform;
@@ -107,6 +139,10 @@ function transformOne(trace, state, attributeSet) {
107139
return self.indexOf(g) === i;
108140
});
109141

142+
if(!(Array.isArray(groups)) || groups.length === 0) {
143+
return trace;
144+
}
145+
110146
var newData = new Array(groupNames.length);
111147
var len = groups.length;
112148

@@ -115,14 +151,6 @@ function transformOne(trace, state, attributeSet) {
115151
var arrayAttributes = attributeSet
116152
.filter(function(array) {return Array.isArray(getDeepProp(trace, array));});
117153

118-
var initializeArray = function(newTrace, a) {
119-
setDeepProp(newTrace, a, []);
120-
};
121-
122-
var pasteArray = function(newTrace, trace, j, a) {
123-
getDeepProp(newTrace, a).push(getDeepProp(trace, a)[j]);
124-
};
125-
126154
// fixme the O(n**3) complexity
127155
for(var i = 0; i < groupNames.length; i++) {
128156
var groupName = groupNames[i];
@@ -148,27 +176,3 @@ function transformOne(trace, state, attributeSet) {
148176

149177
return newData;
150178
}
151-
152-
function getDeepProp(thing, propArray) {
153-
var result = thing;
154-
var i;
155-
for(i = 0; i < propArray.length; i++) {
156-
result = result[propArray[i]];
157-
if(result === void(0)) {
158-
return result;
159-
}
160-
}
161-
return result;
162-
}
163-
164-
function setDeepProp(thing, propArray, value) {
165-
var current = thing;
166-
var i;
167-
for(i = 0; i < propArray.length - 1; i++) {
168-
if(current[propArray[i]] === void(0)) {
169-
current[propArray[i]] = {};
170-
}
171-
current = current[propArray[i]];
172-
}
173-
current[propArray[propArray.length - 1]] = value;
174-
}

‎test/jasmine/tests/transform_groupby_test.js‎

Lines changed: 60 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -271,14 +271,8 @@ describe('groupby', function() {
271271
expect(gd.data[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
272272
expect(gd.data[0].y).toEqual([1, 2, 3, 1, 2, 3, 1]);
273273

274-
expect(gd._fullData.length).toEqual(0); // fixme: it passes with 0; shouldn't it be 1? (one implied group)
275-
276-
/* since the array is of zero length, the below items are obv. meaningless to test
277-
expect(gd._fullData[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
278-
expect(gd._fullData[0].y).toEqual([1, 2, 3, 1, 2, 3, 1]);
279-
*/
280-
281-
assertDims([]); // fixme: same thing, looks like zero dimensionality
274+
expect(gd._fullData.length).toEqual(1);
275+
assertDims([7]);
282276

283277
done();
284278
});
@@ -493,19 +487,6 @@ describe('groupby', function() {
493487
}]
494488
}];
495489

496-
var mockData6 = [{
497-
mode: 'markers+lines',
498-
ids: ['q', 'w', 'r', 't', 'y', 'u', 'i'],
499-
x: [1, -1, -2, 0, 1, 2, 3],
500-
y: [0, 1, 2, 3, 5, 4, 6],
501-
marker: {line: {width: [4, 2, 4, 2, 2, 3, 3]}},
502-
transforms: [{
503-
type: 'groupby',
504-
groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
505-
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
506-
}]
507-
}];
508-
509490
it('`data` preserves user supplied input but `gd._fullData` reflects the grouping', test(mockData1));
510491

511492
it('passes with lots of attributes and heterogenous attrib presence', test(mockData2));
@@ -525,16 +506,43 @@ describe('groupby', function() {
525506

526507
it('passes with no explicit styling in the group transform at all', test(mockData5));
527508

528-
it('passes with no explicit styling in the group transform at all', test(mockData6));
529-
530509
});
531510

532511
describe('passes with no `groups`', function() {
533512
'use strict';
534513

535-
//afterEach(destroyGraphDiv);
514+
afterEach(destroyGraphDiv);
536515

537-
var mockData = [{
516+
function test(mockData) {
517+
518+
return function(done) {
519+
var data = Lib.extendDeep([], mockData);
520+
521+
var gd = createGraphDiv();
522+
523+
Plotly.plot(gd, data).then(function() {
524+
525+
expect(gd.data.length).toEqual(1);
526+
expect(gd.data[0].ids).toEqual(['q', 'w', 'r', 't', 'y', 'u', 'i']);
527+
expect(gd.data[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
528+
expect(gd.data[0].y).toEqual([0, 1, 2, 3, 5, 4, 6]);
529+
expect(gd.data[0].marker.line.width).toEqual([4, 2, 4, 2, 2, 3, 3]);
530+
531+
expect(gd._fullData.length).toEqual(1);
532+
533+
expect(gd._fullData[0].ids).toEqual(['q', 'w', 'r', 't', 'y', 'u', 'i']);
534+
expect(gd._fullData[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
535+
expect(gd._fullData[0].y).toEqual([0, 1, 2, 3, 5, 4, 6]);
536+
expect(gd._fullData[0].marker.line.width).toEqual([4, 2, 4, 2, 2, 3, 3]);
537+
538+
assertDims([7]);
539+
540+
done();
541+
});
542+
};
543+
}
544+
545+
var mockData0 = [{
538546
mode: 'markers+lines',
539547
ids: ['q', 'w', 'r', 't', 'y', 'u', 'i'],
540548
x: [1, -1, -2, 0, 1, 2, 3],
@@ -547,38 +555,35 @@ describe('groupby', function() {
547555
}]
548556
}];
549557

550-
fit('passes', function(done) {
551-
var data = Lib.extendDeep([], mockData);
552-
553-
var gd = createGraphDiv();
554-
555-
Plotly.plot(gd, data).then(function() {
556-
557-
/*
558-
expect(gd.data.length).toEqual(1);
559-
expect(gd.data[0].ids).toEqual(['q', 'w', 'r', 't', 'y', 'u', 'i']);
560-
expect(gd.data[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
561-
expect(gd.data[0].y).toEqual([0, 1, 2, 3, 5, 4, 6]);
562-
expect(gd.data[0].marker.line.width).toEqual([4, 2, 4, 2, 2, 3, 3]);
563-
564-
expect(gd._fullData.length).toEqual(2);
565-
566-
expect(gd._fullData[0].ids).toEqual(['q', 'w', 't', 'i']);
567-
expect(gd._fullData[0].x).toEqual([1, -1, 0, 3]);
568-
expect(gd._fullData[0].y).toEqual([0, 1, 3, 6]);
569-
expect(gd._fullData[0].marker.line.width).toEqual([4, 2, 2, 3]);
570-
571-
expect(gd._fullData[1].ids).toEqual(['r', 'y', 'u']);
572-
expect(gd._fullData[1].x).toEqual([-2, 1, 2]);
573-
expect(gd._fullData[1].y).toEqual([2, 5, 4]);
574-
expect(gd._fullData[1].marker.line.width).toEqual([4, 2, 3]);
558+
var mockData1 = [{
559+
mode: 'markers+lines',
560+
ids: ['q', 'w', 'r', 't', 'y', 'u', 'i'],
561+
x: [1, -1, -2, 0, 1, 2, 3],
562+
y: [0, 1, 2, 3, 5, 4, 6],
563+
marker: {size: 20, line: {width: [4, 2, 4, 2, 2, 3, 3]}},
564+
transforms: [{
565+
type: 'groupby',
566+
groups: [],
567+
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
568+
}]
569+
}];
575570

576-
assertDims([4, 3]);
577-
*/
571+
var mockData2 = [{
572+
mode: 'markers+lines',
573+
ids: ['q', 'w', 'r', 't', 'y', 'u', 'i'],
574+
x: [1, -1, -2, 0, 1, 2, 3],
575+
y: [0, 1, 2, 3, 5, 4, 6],
576+
marker: {size: 20, line: {width: [4, 2, 4, 2, 2, 3, 3]}},
577+
transforms: [{
578+
type: 'groupby',
579+
groups: null,
580+
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
581+
}]
582+
}];
578583

579-
done();
580-
});
581-
});
584+
it('passes with no groups', test(mockData0));
585+
it('passes with empty groups', test(mockData1));
586+
it('passes with falsey groups', test(mockData2));
582587

583588
});
584589
});

0 commit comments

Comments
 (0)