From 84164e3de2aa6eb04048ec0458265db5e99b829b Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Mon, 3 Jul 2017 14:39:06 -0500 Subject: [PATCH 01/11] Add failing test for filter-groupby interaction --- test/jasmine/tests/transform_multi_test.js | 61 ++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/test/jasmine/tests/transform_multi_test.js b/test/jasmine/tests/transform_multi_test.js index 681c39f3737..b067a389e24 100644 --- a/test/jasmine/tests/transform_multi_test.js +++ b/test/jasmine/tests/transform_multi_test.js @@ -727,3 +727,64 @@ describe('restyle applied on transforms:', function() { }); }); + +describe('supplyDefaults with groupby + filter', function () { + function calcDatatoTrace(calcTrace) { + return calcTrace[0].trace; + } + + function _transform(data, layout) { + var gd = { + data: data, + layout: layout || {} + }; + + Plots.supplyDefaults(gd); + Plots.doCalcdata(gd); + + return gd.calcdata.map(calcDatatoTrace); + } + + it('computes the correct data when filter target blank', function () { + var out = _transform([{ + x: [1, 2, 3, 4, 5, 6, 7], + y: [4, 6, 5, 7, 6, 8, 9], + transforms: [{ + type: "filter", + operation: "<", + value: 6.5 + }, { + type: "groupby", + groups: [1, 1, 1, 2, 2, 2, 2] + }] + }]); + + expect(out[0].x).toEqual([1, 2, 3]); + expect(out[0].y).toEqual([4, 6, 5]); + + expect(out[1].x).toEqual([4, 5, 6]); + expect(out[1].y).toEqual([7, 6, 8]); + }); + + it('computes the correct data when filter target present', function () { + var out = _transform([{ + x: [1, 2, 3, 4, 5, 6, 7], + y: [4, 6, 5, 7, 6, 8, 9], + transforms: [{ + type: "filter", + target: [1, 2, 3, 4, 5, 6, 7], + operation: "<", + value: 6.5 + }, { + type: "groupby", + groups: [1, 1, 1, 2, 2, 2, 2] + }] + }]); + + expect(out[0].x).toEqual([1, 2, 3]); + expect(out[0].y).toEqual([4, 6, 5]); + + expect(out[1].x).toEqual([4, 5, 6]); + expect(out[1].y).toEqual([7, 6, 8]); + }); +}); From 9373680ce94c8db89de719fc088a72e56e92b383 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Tue, 25 Jul 2017 13:01:45 -0700 Subject: [PATCH 02/11] Make transforms apply recursively --- src/plots/plots.js | 33 +++++++++++---- src/transforms/groupby.js | 29 +++++++++++-- test/jasmine/tests/transform_multi_test.js | 48 +++++++++++++++++----- 3 files changed, 89 insertions(+), 21 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 17b22b4f230..f8e0cbb3cbe 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1053,25 +1053,42 @@ plots.supplyTransformDefaults = function(traceIn, traceOut, layout) { }; function applyTransforms(fullTrace, fullData, layout, fullLayout) { - var container = fullTrace.transforms, - dataOut = [fullTrace]; + var dataOut = []; - for(var i = 0; i < container.length; i++) { - var transform = container[i], - _module = transformsRegistry[transform.type]; + // Takes single trace, returns list of expanded traces + function recurse(trace, transformIndex) { + var transformedTraces = [trace]; + var transform = trace.transforms[transformIndex]; + var _module = transformsRegistry[transform.type]; if(_module && _module.transform) { - dataOut = _module.transform(dataOut, { + transformedTraces = _module.transform([trace], { transform: transform, - fullTrace: fullTrace, + fullTrace: trace, fullData: fullData, layout: layout, fullLayout: fullLayout, - transformIndex: i + transformIndex: transformIndex }); } + + for(var i = 0; i < transformedTraces.length; i++) { + if(transformIndex >= transformedTraces[i].transforms.length - 1) { + // If we get here, we've reached the end of the line and this expanded + // trace has no more transforms to apply. That is, it's an output trace + // so we push it onto dataOut. + dataOut.push(transformedTraces[i]); + } else { + // Otherwise there are more transforms to be performed. We don't need + // to worry about the return value since every transform will eventually + // reach the end of the line and push its result onto the output. + recurse(transformedTraces[i], transformIndex + 1); + } + } } + recurse(fullTrace, 0); + return dataOut; } diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 6590c428c26..4db35bada8b 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -137,8 +137,22 @@ function pasteArray(newTrace, trace, j, a) { ); } +// In order for groups to apply correctly to other transform data (e.g. +// a filter transform), we have to break the connection and clone the +// transforms so that each group writes grouped values into a different +// destination. This function does not break the array reference +// connection between the split transforms it creates. That's handled +// ininialize, which creates a new empty array for each arrayAttr. +function cloneTransforms(newTrace) { + var transforms = newTrace.transforms; + newTrace.transforms = []; + for(var j = 0; j < transforms.length; j++) { + newTrace.transforms[j] = Lib.extendDeepNoArrays({}, transforms[j]); + } +} + function transformOne(trace, state) { - var i; + var i, j; var opts = state.transform; var groups = trace.transforms[state.transformIndex].groups; @@ -163,12 +177,14 @@ function transformOne(trace, state) { var newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace); + cloneTransforms(newTrace); + arrayAttrs.forEach(initializeArray.bind(null, newTrace)); - for(var j = 0; j < len; j++) { + for(j = 0; j < len; j++) { if(groups[j] !== groupName) continue; - arrayAttrs.forEach(pasteArray.bind(0, newTrace, trace, j)); + arrayAttrs.forEach(pasteArray.bind(null, newTrace, trace, j)); } newTrace.name = groupName; @@ -180,5 +196,12 @@ function transformOne(trace, state) { newTrace = Lib.extendDeepNoArrays(newTrace, styleLookup[groupName] || {}); } + for(i = 0; i < newData.length; i++) { + var data = newData[i]; + var transforms = data.transforms.slice(); + transforms.splice(state.transformIndex, 1); + data.transforms = transforms; + } + return newData; } diff --git a/test/jasmine/tests/transform_multi_test.js b/test/jasmine/tests/transform_multi_test.js index b067a389e24..c9fbe322db4 100644 --- a/test/jasmine/tests/transform_multi_test.js +++ b/test/jasmine/tests/transform_multi_test.js @@ -728,7 +728,7 @@ describe('restyle applied on transforms:', function() { }); -describe('supplyDefaults with groupby + filter', function () { +describe('supplyDefaults with groupby + filter', function() { function calcDatatoTrace(calcTrace) { return calcTrace[0].trace; } @@ -745,16 +745,16 @@ describe('supplyDefaults with groupby + filter', function () { return gd.calcdata.map(calcDatatoTrace); } - it('computes the correct data when filter target blank', function () { + it('computes the correct data when filter target blank', function() { var out = _transform([{ x: [1, 2, 3, 4, 5, 6, 7], y: [4, 6, 5, 7, 6, 8, 9], transforms: [{ - type: "filter", - operation: "<", + type: 'filter', + operation: '<', value: 6.5 }, { - type: "groupby", + type: 'groupby', groups: [1, 1, 1, 2, 2, 2, 2] }] }]); @@ -766,18 +766,46 @@ describe('supplyDefaults with groupby + filter', function () { expect(out[1].y).toEqual([7, 6, 8]); }); - it('computes the correct data when filter target present', function () { + it('computes the correct data when fiter + groupby', function() { var out = _transform([{ + x: [5, 4, 3], + y: [6, 5, 4], + }, { x: [1, 2, 3, 4, 5, 6, 7], - y: [4, 6, 5, 7, 6, 8, 9], + y: [4, 6, 5, 7, 8, 9, 10], transforms: [{ - type: "filter", + type: 'filter', target: [1, 2, 3, 4, 5, 6, 7], - operation: "<", + operation: '<', value: 6.5 }, { - type: "groupby", + type: 'groupby', + groups: [1, 1, 1, 2, 2, 2, 2] + }] + }]); + + expect(out[0].x).toEqual([5, 4, 3]); + expect(out[0].y).toEqual([6, 5, 4]); + + expect(out[1].x).toEqual([1, 2, 3]); + expect(out[1].y).toEqual([4, 6, 5]); + + expect(out[2].x).toEqual([4, 5, 6]); + expect(out[2].y).toEqual([7, 8, 9]); + }); + + it('computes the correct data when groupby + filter', function() { + var out = _transform([{ + x: [1, 2, 3, 4, 5, 6, 7], + y: [4, 6, 5, 7, 6, 8, 9], + transforms: [{ + type: 'groupby', groups: [1, 1, 1, 2, 2, 2, 2] + }, { + type: 'filter', + target: [1, 2, 3, 4, 5, 6, 7], + operation: '<', + value: 6.5 }] }]); From 9b44b4bd597443eb99cf98828e44800e509ab675 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Tue, 25 Jul 2017 13:03:20 -0700 Subject: [PATCH 03/11] Remove transform hack --- src/transforms/groupby.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 4db35bada8b..28823be6e5d 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -196,12 +196,5 @@ function transformOne(trace, state) { newTrace = Lib.extendDeepNoArrays(newTrace, styleLookup[groupName] || {}); } - for(i = 0; i < newData.length; i++) { - var data = newData[i]; - var transforms = data.transforms.slice(); - transforms.splice(state.transformIndex, 1); - data.transforms = transforms; - } - return newData; } From 7ebee7d792218358f1ac353ff41ef89116e5f49e Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Tue, 25 Jul 2017 13:31:18 -0700 Subject: [PATCH 04/11] Add group + group test and revert src/plots --- src/plots/plots.js | 33 ++++++---------------- test/jasmine/tests/transform_multi_test.js | 24 ++++++++++++++++ 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index f8e0cbb3cbe..17b22b4f230 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1053,42 +1053,25 @@ plots.supplyTransformDefaults = function(traceIn, traceOut, layout) { }; function applyTransforms(fullTrace, fullData, layout, fullLayout) { - var dataOut = []; + var container = fullTrace.transforms, + dataOut = [fullTrace]; - // Takes single trace, returns list of expanded traces - function recurse(trace, transformIndex) { - var transformedTraces = [trace]; - var transform = trace.transforms[transformIndex]; - var _module = transformsRegistry[transform.type]; + for(var i = 0; i < container.length; i++) { + var transform = container[i], + _module = transformsRegistry[transform.type]; if(_module && _module.transform) { - transformedTraces = _module.transform([trace], { + dataOut = _module.transform(dataOut, { transform: transform, - fullTrace: trace, + fullTrace: fullTrace, fullData: fullData, layout: layout, fullLayout: fullLayout, - transformIndex: transformIndex + transformIndex: i }); } - - for(var i = 0; i < transformedTraces.length; i++) { - if(transformIndex >= transformedTraces[i].transforms.length - 1) { - // If we get here, we've reached the end of the line and this expanded - // trace has no more transforms to apply. That is, it's an output trace - // so we push it onto dataOut. - dataOut.push(transformedTraces[i]); - } else { - // Otherwise there are more transforms to be performed. We don't need - // to worry about the return value since every transform will eventually - // reach the end of the line and push its result onto the output. - recurse(transformedTraces[i], transformIndex + 1); - } - } } - recurse(fullTrace, 0); - return dataOut; } diff --git a/test/jasmine/tests/transform_multi_test.js b/test/jasmine/tests/transform_multi_test.js index c9fbe322db4..30677886af6 100644 --- a/test/jasmine/tests/transform_multi_test.js +++ b/test/jasmine/tests/transform_multi_test.js @@ -815,4 +815,28 @@ describe('supplyDefaults with groupby + filter', function() { expect(out[1].x).toEqual([4, 5, 6]); expect(out[1].y).toEqual([7, 6, 8]); }); + + it('computes the correct data when groupby + groupby', function() { + var out = _transform([{ + x: [1, 2, 3, 4, 5, 6, 7, 8], + y: [4, 6, 5, 7, 6, 8, 9, 10], + transforms: [{ + type: 'groupby', + groups: [1, 1, 1, 1, 2, 2, 2, 2] + }, { + type: 'groupby', + groups: [3, 4, 3, 4, 3, 4, 3, 5], + }] + }]); + // | | | | | | | | + // v v v v v v v v + // Trace number: 0 1 0 1 2 3 2 4 + + expect(out.length).toEqual(5); + expect(out[0].x).toEqual([1, 3]); + expect(out[1].x).toEqual([2, 4]); + expect(out[2].x).toEqual([5, 7]); + expect(out[3].x).toEqual([6]); + expect(out[4].x).toEqual([8]); + }); }); From c74248781580522dbf3ad1831f07f835bd0eb8d1 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Tue, 25 Jul 2017 13:42:49 -0700 Subject: [PATCH 05/11] Add groupby + groupby + filter test --- test/jasmine/tests/transform_multi_test.js | 37 +++++++++++++++++++--- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/test/jasmine/tests/transform_multi_test.js b/test/jasmine/tests/transform_multi_test.js index 30677886af6..d8f69532010 100644 --- a/test/jasmine/tests/transform_multi_test.js +++ b/test/jasmine/tests/transform_multi_test.js @@ -745,7 +745,7 @@ describe('supplyDefaults with groupby + filter', function() { return gd.calcdata.map(calcDatatoTrace); } - it('computes the correct data when filter target blank', function() { + it('filter + groupby with blank target', function() { var out = _transform([{ x: [1, 2, 3, 4, 5, 6, 7], y: [4, 6, 5, 7, 6, 8, 9], @@ -766,7 +766,7 @@ describe('supplyDefaults with groupby + filter', function() { expect(out[1].y).toEqual([7, 6, 8]); }); - it('computes the correct data when fiter + groupby', function() { + it('fiter + groupby', function() { var out = _transform([{ x: [5, 4, 3], y: [6, 5, 4], @@ -794,7 +794,7 @@ describe('supplyDefaults with groupby + filter', function() { expect(out[2].y).toEqual([7, 8, 9]); }); - it('computes the correct data when groupby + filter', function() { + it('groupby + filter', function() { var out = _transform([{ x: [1, 2, 3, 4, 5, 6, 7], y: [4, 6, 5, 7, 6, 8, 9], @@ -816,7 +816,7 @@ describe('supplyDefaults with groupby + filter', function() { expect(out[1].y).toEqual([7, 6, 8]); }); - it('computes the correct data when groupby + groupby', function() { + it('groupby + groupby', function() { var out = _transform([{ x: [1, 2, 3, 4, 5, 6, 7, 8], y: [4, 6, 5, 7, 6, 8, 9, 10], @@ -839,4 +839,33 @@ describe('supplyDefaults with groupby + filter', function() { expect(out[3].x).toEqual([6]); expect(out[4].x).toEqual([8]); }); + + it('groupby + groupby + filter', function() { + var out = _transform([{ + x: [1, 2, 3, 4, 5, 6, 7, 8], + y: [4, 6, 5, 7, 6, 8, 9, 10], + transforms: [{ + type: 'groupby', + groups: [1, 1, 1, 1, 2, 2, 2, 2] + }, { + type: 'groupby', + groups: [3, 4, 3, 4, 3, 4, 3, 5], + }, { + type: 'filter', + target: [1, 2, 3, 4, 5, 6, 7, 8], + operation: '<', + value: 4.5 + }] + }]); + // | | | | | | | | + // v v v v v v v v + // Trace number: 0 1 0 1 2 3 2 4 + + expect(out.length).toEqual(5); + expect(out[0].x).toEqual([1, 3]); + expect(out[1].x).toEqual([2, 4]); + expect(out[2].x).toEqual([]); + expect(out[3].x).toEqual([]); + expect(out[4].x).toEqual([]); + }); }); From 566883026966ff2a3d7ad2f23b90f8d98aafe668 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Tue, 25 Jul 2017 13:51:09 -0700 Subject: [PATCH 06/11] Typo fix in groupby comments --- src/transforms/groupby.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 28823be6e5d..30a768796ae 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -141,8 +141,8 @@ function pasteArray(newTrace, trace, j, a) { // a filter transform), we have to break the connection and clone the // transforms so that each group writes grouped values into a different // destination. This function does not break the array reference -// connection between the split transforms it creates. That's handled -// ininialize, which creates a new empty array for each arrayAttr. +// connection between the split transforms it creates. That's handled in +// initialize, which creates a new empty array for each arrayAttr. function cloneTransforms(newTrace) { var transforms = newTrace.transforms; newTrace.transforms = []; From 695ad68d72e3257131ac03ab9625b653c114d574 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Tue, 25 Jul 2017 13:54:18 -0700 Subject: [PATCH 07/11] Add filter + filter test --- test/jasmine/tests/transform_multi_test.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/jasmine/tests/transform_multi_test.js b/test/jasmine/tests/transform_multi_test.js index d8f69532010..0592b177b8c 100644 --- a/test/jasmine/tests/transform_multi_test.js +++ b/test/jasmine/tests/transform_multi_test.js @@ -868,4 +868,25 @@ describe('supplyDefaults with groupby + filter', function() { expect(out[3].x).toEqual([]); expect(out[4].x).toEqual([]); }); + + it('fiter + filter', function() { + var out = _transform([{ + x: [1, 2, 3, 4, 5, 6, 7], + y: [4, 6, 5, 7, 8, 9, 10], + transforms: [{ + type: 'filter', + target: [1, 2, 3, 4, 5, 6, 7], + operation: '<', + value: 6.5 + }, { + type: 'filter', + target: [1, 2, 3, 4, 5, 6, 7], + operation: '>', + value: 1.5 + }] + }]); + + expect(out[0].x).toEqual([2, 3, 4, 5, 6]); + expect(out[0].y).toEqual([6, 5, 7, 8, 9]); + }); }); From 1544e71bf93331da91ad91a97477909e36600f5a Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 26 Jul 2017 09:59:38 -0700 Subject: [PATCH 08/11] Optimize and refactor groupby --- src/transforms/groupby.js | 83 +++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 30a768796ae..6c89b5960d9 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -116,43 +116,23 @@ exports.supplyDefaults = function(transformIn) { * array of transformed traces */ exports.transform = function(data, state) { + var newTraces, i, j; var newData = []; - for(var i = 0; i < data.length; i++) { - newData = newData.concat(transformOne(data[i], state)); + for(i = 0; i < data.length; i++) { + newTraces = transformOne(data[i], state); + + for(j = 0; j < newTraces.length; j++) { + newData.push(newTraces[j]); + } } return newData; }; -function initializeArray(newTrace, a) { - Lib.nestedProperty(newTrace, a).set([]); -} - -function pasteArray(newTrace, trace, j, a) { - Lib.nestedProperty(newTrace, a).set( - Lib.nestedProperty(newTrace, a).get().concat([ - Lib.nestedProperty(trace, a).get()[j] - ]) - ); -} - -// In order for groups to apply correctly to other transform data (e.g. -// a filter transform), we have to break the connection and clone the -// transforms so that each group writes grouped values into a different -// destination. This function does not break the array reference -// connection between the split transforms it creates. That's handled in -// initialize, which creates a new empty array for each arrayAttr. -function cloneTransforms(newTrace) { - var transforms = newTrace.transforms; - newTrace.transforms = []; - for(var j = 0; j < transforms.length; j++) { - newTrace.transforms[j] = Lib.extendDeepNoArrays({}, transforms[j]); - } -} function transformOne(trace, state) { - var i, j; + var i, j, k, attr, srcArray, groupName, newTrace, transforms; var opts = state.transform; var groups = trace.transforms[state.transformIndex].groups; @@ -172,22 +152,31 @@ function transformOne(trace, state) { styleLookup[styles[i].target] = styles[i].value; } - for(i = 0; i < groupNames.length; i++) { - var groupName = groupNames[i]; - - var newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace); - - cloneTransforms(newTrace); + var newDataByGroup = {}; - arrayAttrs.forEach(initializeArray.bind(null, newTrace)); + for(i = 0; i < groupNames.length; i++) { + groupName = groupNames[i]; - for(j = 0; j < len; j++) { - if(groups[j] !== groupName) continue; + // Start with a deep extend that just copies array references. + newTrace = newData[i] = newDataByGroup[groupName] = Lib.extendDeepNoArrays({}, trace); + newTrace.name = groupName; - arrayAttrs.forEach(pasteArray.bind(null, newTrace, trace, j)); + // In order for groups to apply correctly to other transform data (e.g. + // a filter transform), we have to break the connection and clone the + // transforms so that each group writes grouped values into a different + // destination. This function does not break the array reference + // connection between the split transforms it creates. That's handled in + // initialize, which creates a new empty array for each arrayAttr. + transforms = newTrace.transforms; + newTrace.transforms = []; + for(j = 0; j < transforms.length; j++) { + newTrace.transforms[j] = Lib.extendDeepNoArrays({}, transforms[j]); } - newTrace.name = groupName; + // Initialize empty arrays for the arrayAttrs, to be split in the next step + for(j = 0; j < arrayAttrs.length; j++) { + Lib.nestedProperty(newTrace, arrayAttrs[j]).set([]); + } Plots.clearExpandedTraceDefaultColors(newTrace); @@ -196,5 +185,21 @@ function transformOne(trace, state) { newTrace = Lib.extendDeepNoArrays(newTrace, styleLookup[groupName] || {}); } + + // For each array attribute including those nested inside this and other + // transforms (small note that we technically only need to do this for + // transforms that have not yet been applied): + for(k = 0; k < arrayAttrs.length; k++) { + attr = arrayAttrs[k]; + + // Get the input data: + srcArray = Lib.nestedProperty(trace, attr).get(); + + // And push each value onto the appropriate destination for this group: + for(j = 0; j < len; j++) { + Lib.nestedProperty(newDataByGroup[groups[j]], attr).get().push(srcArray[j]); + } + } + return newData; } From 9f0600afb0db1c34e2f62e7364a75dc98d665f9d Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 26 Jul 2017 10:53:10 -0700 Subject: [PATCH 09/11] Style groups *after* computing data --- src/transforms/groupby.js | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 6c89b5960d9..0a53989045a 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -137,7 +137,7 @@ function transformOne(trace, state) { var groups = trace.transforms[state.transformIndex].groups; if(!(Array.isArray(groups)) || groups.length === 0) { - return trace; + return [trace]; } var groupNames = Lib.filterUnique(groups), @@ -177,12 +177,6 @@ function transformOne(trace, state) { for(j = 0; j < arrayAttrs.length; j++) { Lib.nestedProperty(newTrace, arrayAttrs[j]).set([]); } - - Plots.clearExpandedTraceDefaultColors(newTrace); - - // there's no need to coerce styleLookup[groupName] here - // as another round of supplyDefaults is done on the transformed traces - newTrace = Lib.extendDeepNoArrays(newTrace, styleLookup[groupName] || {}); } @@ -201,5 +195,16 @@ function transformOne(trace, state) { } } + for(i = 0; i < groupNames.length; i++) { + groupName = groupNames[i]; + newTrace = newData[i]; + + Plots.clearExpandedTraceDefaultColors(newTrace); + + // there's no need to coerce styleLookup[groupName] here + // as another round of supplyDefaults is done on the transformed traces + newTrace = Lib.extendDeepNoArrays(newTrace, styleLookup[groupName] || {}); + } + return newData; } From 364f8f746053b49147236d7e258c000bddced2fc Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 26 Jul 2017 11:51:31 -0700 Subject: [PATCH 10/11] Avoid extra nestedProperty creation in groupby --- src/transforms/groupby.js | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 0a53989045a..b11461b1472 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -132,7 +132,8 @@ exports.transform = function(data, state) { function transformOne(trace, state) { - var i, j, k, attr, srcArray, groupName, newTrace, transforms; + var i, j, k, attr, srcArray, groupName, newTrace, transforms, arrayLookup; + var opts = state.transform; var groups = trace.transforms[state.transformIndex].groups; @@ -152,13 +153,15 @@ function transformOne(trace, state) { styleLookup[styles[i].target] = styles[i].value; } - var newDataByGroup = {}; + // An index to map group name --> expanded trace index + var groupIndex = {}; for(i = 0; i < groupNames.length; i++) { groupName = groupNames[i]; + groupIndex[groupName] = i; // Start with a deep extend that just copies array references. - newTrace = newData[i] = newDataByGroup[groupName] = Lib.extendDeepNoArrays({}, trace); + newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace); newTrace.name = groupName; // In order for groups to apply correctly to other transform data (e.g. @@ -179,19 +182,24 @@ function transformOne(trace, state) { } } - // For each array attribute including those nested inside this and other // transforms (small note that we technically only need to do this for // transforms that have not yet been applied): for(k = 0; k < arrayAttrs.length; k++) { attr = arrayAttrs[k]; + // Cache all the arrays to which we'll push: + for(j = 0, arrayLookup = []; j < groupNames.length; j++) { + arrayLookup[j] = Lib.nestedProperty(newData[j], attr).get(); + } + // Get the input data: srcArray = Lib.nestedProperty(trace, attr).get(); - // And push each value onto the appropriate destination for this group: + // Send each data point to the appropriate expanded trace: for(j = 0; j < len; j++) { - Lib.nestedProperty(newDataByGroup[groups[j]], attr).get().push(srcArray[j]); + // Map group data --> trace index --> array and push data onto it + arrayLookup[groupIndex[groups[j]]].push(srcArray[j]); } } From 88b5673f8f1971f6540f62b93831b2d0177fccff Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 26 Jul 2017 11:57:58 -0700 Subject: [PATCH 11/11] Select less deliberately confusing name for variable --- src/transforms/groupby.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index b11461b1472..92e00d17fb3 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -154,11 +154,11 @@ function transformOne(trace, state) { } // An index to map group name --> expanded trace index - var groupIndex = {}; + var indexLookup = {}; for(i = 0; i < groupNames.length; i++) { groupName = groupNames[i]; - groupIndex[groupName] = i; + indexLookup[groupName] = i; // Start with a deep extend that just copies array references. newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace); @@ -199,7 +199,7 @@ function transformOne(trace, state) { // Send each data point to the appropriate expanded trace: for(j = 0; j < len; j++) { // Map group data --> trace index --> array and push data onto it - arrayLookup[groupIndex[groups[j]]].push(srcArray[j]); + arrayLookup[indexLookup[groups[j]]].push(srcArray[j]); } }