From e555e0ab6438cf298fbee3c925435246178b2d61 Mon Sep 17 00:00:00 2001 From: bpostlethwaite Date: Wed, 20 Sep 2017 14:26:44 -0400 Subject: [PATCH 1/3] missing aggregations array no longer throws - fix #2024 --- src/transforms/aggregate.js | 32 +++++++++---------- .../jasmine/tests/transform_aggregate_test.js | 20 ++++++++++++ 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/src/transforms/aggregate.js b/src/transforms/aggregate.js index 932e54f4ccc..a2722c7f549 100644 --- a/src/transforms/aggregate.js +++ b/src/transforms/aggregate.js @@ -155,7 +155,7 @@ exports.supplyDefaults = function(transformIn, traceOut) { arrayAttrs[groups] = 0; } - var aggregationsIn = transformIn.aggregations; + var aggregationsIn = transformIn.aggregations || []; var aggregationsOut = transformOut.aggregations = new Array(aggregationsIn.length); var aggregationOut; @@ -163,23 +163,21 @@ exports.supplyDefaults = function(transformIn, traceOut) { return Lib.coerce(aggregationsIn[i], aggregationOut, aggAttrs, attr, dflt); } - if(aggregationsIn) { - for(i = 0; i < aggregationsIn.length; i++) { - aggregationOut = {}; - var target = coercei('target'); - var func = coercei('func'); - var enabledi = coercei('enabled'); - - // add this aggregation to the output only if it's the first instance - // of a valid target attribute - or an unused target attribute with "count" - if(enabledi && target && (arrayAttrs[target] || (func === 'count' && arrayAttrs[target] === undefined))) { - if(func === 'stddev') coercei('funcmode'); - - arrayAttrs[target] = 0; - aggregationsOut[i] = aggregationOut; - } - else aggregationsOut[i] = {enabled: false}; + for(i = 0; i < aggregationsIn.length; i++) { + aggregationOut = {}; + var target = coercei('target'); + var func = coercei('func'); + var enabledi = coercei('enabled'); + + // add this aggregation to the output only if it's the first instance + // of a valid target attribute - or an unused target attribute with "count" + if(enabledi && target && (arrayAttrs[target] || (func === 'count' && arrayAttrs[target] === undefined))) { + if(func === 'stddev') coercei('funcmode'); + + arrayAttrs[target] = 0; + aggregationsOut[i] = aggregationOut; } + else aggregationsOut[i] = {enabled: false}; } // any array attributes we haven't yet covered, fill them with the default aggregation diff --git a/test/jasmine/tests/transform_aggregate_test.js b/test/jasmine/tests/transform_aggregate_test.js index 0377963138b..15bca9f1d21 100644 --- a/test/jasmine/tests/transform_aggregate_test.js +++ b/test/jasmine/tests/transform_aggregate_test.js @@ -190,6 +190,26 @@ describe('aggregate', function() { expect(traceOut.marker.size).toEqual([10, 20]); }); + // Regression test - throws before fix: + // https://github.com/plotly/plotly.js/issues/2024 + it('can handle case where aggregation array is missing', function() { + Plotly.newPlot(gd, [{ + x: [1, 2, 3, 4, 5], + y: [2, 4, 6, 8, 10], + marker: {size: [10, 10, 20, 20, 10]}, + transforms: [{ + type: 'aggregate', + groups: 'marker.size' + }] + }]); + + var traceOut = gd._fullData[0]; + + expect(traceOut.x).toEqual([1, 3]); + expect(traceOut.y).toEqual([2, 6]); + expect(traceOut.marker.size).toEqual([10, 20]); + }); + it('handles median, mode, rms, & stddev for numeric data', function() { // again, nothing is going to barf with non-numeric data, but sometimes it // won't make much sense. From 75e6e48ea88a848006a7a593952916d9c2b6a4d1 Mon Sep 17 00:00:00 2001 From: bpostlethwaite Date: Fri, 22 Sep 2017 13:56:20 -0400 Subject: [PATCH 2/3] add and test aggregation back _index. This necessitated a change in plots.js to prevent the supply defaults step running twice with the second run acting on fullData rather then userData. --- src/plots/plots.js | 14 +++++++- src/transforms/aggregate.js | 7 ++-- .../jasmine/tests/transform_aggregate_test.js | 36 +++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index e8ff7e50bc8..242a49d4ecb 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1039,9 +1039,21 @@ plots.supplyTransformDefaults = function(traceIn, traceOut, layout) { _module = transformsRegistry[type], transformOut; + /* + * Supply defaults may run twice. First pass runs all supply defaults steps + * and adds the _module to any output transforms. + * If transforms exist another pass is run so that any generated traces also + * go through supply defaults. This has the effect of rerunning + * supplyTransforms. If the transform does not have a `transform` function + * it could not have generated any new traces and the second stage is + * unnecessary. We detect this case with the following variables. + */ + var isSecondStage = transformIn._module && transformIn._module === _module, + doSecondStage = _module && typeof _module.transform === 'function'; + if(!_module) Lib.warn('Unrecognized transform type ' + type + '.'); - if(_module && _module.supplyDefaults) { + if(_module && _module.supplyDefaults && (!isSecondStage || doSecondStage)) { transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn); transformOut.type = type; transformOut._module = _module; diff --git a/src/transforms/aggregate.js b/src/transforms/aggregate.js index a2722c7f549..3000404c080 100644 --- a/src/transforms/aggregate.js +++ b/src/transforms/aggregate.js @@ -164,7 +164,7 @@ exports.supplyDefaults = function(transformIn, traceOut) { } for(i = 0; i < aggregationsIn.length; i++) { - aggregationOut = {}; + aggregationOut = {_index: i}; var target = coercei('target'); var func = coercei('func'); var enabledi = coercei('enabled'); @@ -177,7 +177,7 @@ exports.supplyDefaults = function(transformIn, traceOut) { arrayAttrs[target] = 0; aggregationsOut[i] = aggregationOut; } - else aggregationsOut[i] = {enabled: false}; + else aggregationsOut[i] = {enabled: false, _index: i}; } // any array attributes we haven't yet covered, fill them with the default aggregation @@ -186,7 +186,8 @@ exports.supplyDefaults = function(transformIn, traceOut) { aggregationsOut.push({ target: arrayAttrArray[i], func: aggAttrs.func.dflt, - enabled: true + enabled: true, + _index: -1 }); } } diff --git a/test/jasmine/tests/transform_aggregate_test.js b/test/jasmine/tests/transform_aggregate_test.js index 15bca9f1d21..5da7dfaa063 100644 --- a/test/jasmine/tests/transform_aggregate_test.js +++ b/test/jasmine/tests/transform_aggregate_test.js @@ -245,4 +245,40 @@ describe('aggregate', function() { expect(traceOut.marker.line.width).toBeCloseToArray([0.5, 0], 5); expect(traceOut.marker.color).toBeCloseToArray([Math.sqrt(1 / 3), 0], 5); }); + + it('links fullData aggregations to userData via _index', function() { + Plotly.newPlot(gd, [{ + x: [1, 2, 3, 4, 5], + y: [2, 4, 6, 8, 10], + marker: { + size: [10, 10, 20, 20, 10], + color: ['red', 'green', 'blue', 'yellow', 'white'] + }, + transforms: [{ + type: 'aggregate', + groups: 'marker.size', + aggregations: [ + {target: 'x', func: 'sum'}, + {target: 'x', func: 'avg'}, + {target: 'y', func: 'avg'}, + ] + }] + }]); + + var traceOut = gd._fullData[0]; + var fullAggregation = traceOut.transforms[0]; + var fullAggregations = fullAggregation.aggregations; + var enabledAggregations = fullAggregations.filter(function(agg) { + return agg.enabled; + }); + + expect(enabledAggregations[0].target).toEqual('x'); + expect(enabledAggregations[0]._index).toEqual(0); + + expect(enabledAggregations[1].target).toEqual('y'); + expect(enabledAggregations[1]._index).toEqual(2); + + expect(enabledAggregations[2].target).toEqual('marker.color'); + expect(enabledAggregations[2]._index).toEqual(-1); + }); }); From ec0a65f0f6dee798e912f61ea4180e5d0969958c Mon Sep 17 00:00:00 2001 From: bpostlethwaite Date: Thu, 28 Sep 2017 15:24:08 -0400 Subject: [PATCH 3/3] documentation and variable name improvements. Thanks @rreusser --- src/plots/plots.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 242a49d4ecb..e27a7710fbc 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1044,16 +1044,16 @@ plots.supplyTransformDefaults = function(traceIn, traceOut, layout) { * and adds the _module to any output transforms. * If transforms exist another pass is run so that any generated traces also * go through supply defaults. This has the effect of rerunning - * supplyTransforms. If the transform does not have a `transform` function - * it could not have generated any new traces and the second stage is - * unnecessary. We detect this case with the following variables. + * supplyTransformDefaults. If the transform does not have a `transform` + * function it could not have generated any new traces and the second stage + * is unnecessary. We detect this case with the following variables. */ - var isSecondStage = transformIn._module && transformIn._module === _module, - doSecondStage = _module && typeof _module.transform === 'function'; + var isFirstStage = !(transformIn._module && transformIn._module === _module), + doLaterStages = _module && typeof _module.transform === 'function'; if(!_module) Lib.warn('Unrecognized transform type ' + type + '.'); - if(_module && _module.supplyDefaults && (!isSecondStage || doSecondStage)) { + if(_module && _module.supplyDefaults && (isFirstStage || doLaterStages)) { transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn); transformOut.type = type; transformOut._module = _module;