Repository: vega/vega-lite
Pull request #626 (2015-10-08 15:39:19)

Simplify stack code
#276

- move stack’s stacked data transform to `data.stack`
- add `encoding.stack` so we do not have to pass `stack` around

- TODO in future PRs 
  - consider caching stack as encoding’s property to avoid redundant computation
  - remove `stack.js` and move the same logic to `marks.js` to reduce side effect
  - consider if there is a way to avoid creating stacked data, which perform redundant calculation with scales in each facet.  (maybe signal could work)


Patch:
diff --git a/src/Encoding.js b/src/Encoding.js
index f8e4be252..3aaec7d39 100644
--- a/src/Encoding.js
+++ b/src/Encoding.js
@@ -260,11 +260,38 @@ module.exports = (function() {
       spec.encoding.color;
   };
 
-  proto.isStack = function() {
-    // FIXME update this once we have control for stack ...
-    return (this.is('bar') || this.is('area')) && this.has('color');
+  /**
+   * Check if the encoding should be stacked and return the stack dimenstion and value fields.
+   * @return {Object} An object containing two properties:
+   * - dimension - the dimension field
+   * - value - the value field
+   */
+  proto.stack = function() {
+    if ((this.is('bar') || this.is('area')) &&
+        (this.has('color') || this.has('detail')) &&
+        // TODO(#) update this once we have control for stack ...
+        this.isAggregate()) {
+
+      var isXMeasure = this.isMeasure(X);
+      var isYMeasure = this.isMeasure(Y);
+
+      if (isXMeasure && !isYMeasure) {
+        return {
+          dimension: Y,
+          value: X
+        };
+      } else if (isYMeasure && !isXMeasure) {
+        return {
+          dimension: X,
+          value: Y
+        };
+      }
+    }
+    return null; // no stack encoding
   };
 
+
+
   proto.details = function() {
     var encoding = this;
     return this.reduce(function(refs, field, encType) {
diff --git a/src/compiler/compiler.js b/src/compiler/compiler.js
index 5dafd0dc4..d92ddc085 100644
--- a/src/compiler/compiler.js
+++ b/src/compiler/compiler.js
@@ -80,22 +80,28 @@ compiler.compileEncoding = function (encoding, stats) {
     mdefs = group.marks = marks.def(encoding, layout, style, stats),
     mdef = mdefs[mdefs.length - 1];  // TODO: remove this dirty hack by refactoring the whole flow
 
+  var stack = encoding.stack();
+  if (stack) {
+    // modify mdef.{from,properties}
+    compiler.stack(encoding, mdef, stack);
+  }
+
   var lineType = marks[encoding.marktype()].line;
 
   // handle subfacets
-
-  var details = encoding.details(),
-    stack = encoding.isAggregate() && details.length > 0 && compiler.stack(spec.data, encoding, mdef); // modify spec.data, mdef.{from,properties}
+  var details = encoding.details();
 
   if (details.length > 0 && lineType) {
     //subfacet to group stack / line together in one group
-    compiler.subfacet(group, mdef, details, stack, encoding);
+    compiler.subfacet(group, mdef, details, encoding);
   }
 
   // auto-sort line/area values
   if (lineType && encoding.config('autoSortLine')) {
     var f = (encoding.isMeasure(X) && encoding.isDimension(Y)) ? Y : X;
-    if (!mdef.from) mdef.from = {};
+    if (!mdef.from) {
+      mdef.from = {};
+    }
     // TODO: why - ?
     mdef.from.transform = [{type: 'sort', by: '-' + encoding.fieldRef(f)}];
   }
@@ -107,14 +113,17 @@ compiler.compileEncoding = function (encoding, stats) {
 
   // Small Multiples
   if (encoding.has(ROW) || encoding.has(COL)) {
-    spec = compiler.facet(group, encoding, layout, spec, singleScaleNames, stack, stats);
+    spec = compiler.facet(group, encoding, layout, spec, singleScaleNames, stats);
     spec.legends = legend.defs(encoding, style);
   } else {
-    group.scales = scale.defs(singleScaleNames, encoding, layout, stats, {stack: stack});
-
+    group.scales = scale.defs(singleScaleNames, encoding, layout, stats);
     group.axes = [];
-    if (encoding.has(X)) group.axes.push(axis.def(X, encoding, layout, stats));
-    if (encoding.has(Y)) group.axes.push(axis.def(Y, encoding, layout, stats));
+    if (encoding.has(X)) {
+      group.axes.push(axis.def(X, encoding, layout, stats));
+    }
+    if (encoding.has(Y)) {
+      group.axes.push(axis.def(Y, encoding, layout, stats));
+    }
 
     group.legends = legend.defs(encoding, style);
   }
diff --git a/src/compiler/data.js b/src/compiler/data.js
index 51d65e2c9..61a4f729e 100644
--- a/src/compiler/data.js
+++ b/src/compiler/data.js
@@ -22,13 +22,21 @@ function data(encoding, stats) {
   var def = [data.raw(encoding, stats)];
 
   var aggregate = data.aggregate(encoding);
-  if (aggregate) def.push(data.aggregate(encoding));
+  if (aggregate) {
+    def.push(data.aggregate(encoding));
+  }
 
   // TODO add "having" filter here
 
   // append non-positive filter at the end for the data table
   data.filterNonPositive(def[def.length - 1], encoding);
 
+  // Stack
+  var stack = encoding.stack();
+  if (stack) {
+    def.push(data.stack(encoding, stack));
+  }
+
   return def;
 }
 
@@ -202,6 +210,38 @@ data.aggregate = function(encoding) {
   return null;
 };
 
+/**
+ * Add stacked data source, for feeding the shared scale.
+ */
+data.stack = function(encoding, stack) {
+  var dim = stack.dimension;
+  var val = stack.value;
+  var facets = encoding.facets();
+
+  var stacked = {
+    name: STACKED,
+    source: encoding.dataTable(),
+    transform: [{
+      type: 'aggregate',
+      groupby: [encoding.fieldRef(dim)].concat(facets), // dim and other facets
+      summarize: [{ops: ['sum'], field: encoding.fieldRef(val)}]
+    }]
+  };
+
+  if (facets && facets.length > 0) {
+    stacked.transform.push({ //calculate max for each facet
+      type: 'aggregate',
+      groupby: facets,
+      summarize: [{
+        ops: ['max'],
+        // we want max of sum from above transform
+        field: encoding.fieldRef(val, {prefn: 'sum_'})
+      }]
+    });
+  }
+  return stacked;
+};
+
 data.filterNonPositive = function(dataTable, encoding) {
   encoding.forEach(function(encDef, encType) {
     if (encoding.scale(encType).type === 'log') {
diff --git a/src/compiler/facet.js b/src/compiler/facet.js
index 249ba15d3..c0e485bab 100644
--- a/src/compiler/facet.js
+++ b/src/compiler/facet.js
@@ -29,7 +29,7 @@ function groupdef(name, opt) {
   };
 }
 
-function faceting(group, encoding, layout, spec, singleScaleNames, stack, stats) {
+function faceting(group, encoding, layout, spec, singleScaleNames, stats) {
   var enter = group.properties.enter;
   var facetKeys = [], cellAxes = [], from, axesGrp;
 
@@ -121,7 +121,7 @@ function faceting(group, encoding, layout, spec, singleScaleNames, stack, stats)
     encoding,
     layout,
     stats,
-    {stack: stack, facet: true}
+    true
   )); // row/col scales + cell scales
 
   if (cellAxes.length > 0) {
diff --git a/src/compiler/marks.js b/src/compiler/marks.js
index 55fe2f140..aedbe668b 100644
--- a/src/compiler/marks.js
+++ b/src/compiler/marks.js
@@ -39,7 +39,6 @@ marks.def = function(encoding, layout, style, stats) {
 
 marks.bar = {
   type: 'rect',
-  stack: true,
   prop: bar_props,
   supportedEncoding: {row: 1, col: 1, x: 1, y: 1, size: 1, color: 1}
 };
@@ -54,7 +53,6 @@ marks.line = {
 
 marks.area = {
   type: 'area',
-  stack: true,
   line: true,
   requiredEncoding: ['x', 'y'],
   prop: area_props,
diff --git a/src/compiler/scale.js b/src/compiler/scale.js
index cbde7cbd5..25d7d7645 100644
--- a/src/compiler/scale.js
+++ b/src/compiler/scale.js
@@ -15,17 +15,15 @@ scale.names = function(props) {
   }, {}));
 };
 
-scale.defs = function(names, encoding, layout, stats, opt) {
-  opt = opt || {};
-
+scale.defs = function(names, encoding, layout, stats, facet) {
   return names.reduce(function(a, name) {
     var scaleDef = {
       name: name,
       type: scale.type(name, encoding)
     };
 
-    scaleDef.domain = scale.domain(scaleDef, encoding, stats, opt);
-    scaleDef = scale.range(scaleDef, encoding, layout, stats, opt);
+    scaleDef.domain = scale.domain(scaleDef, encoding, stats, facet);
+    scaleDef = scale.range(scaleDef, encoding, layout, stats);
 
     return (a.push(scaleDef), a);
   }, []);
@@ -48,9 +46,7 @@ scale.type = function(name, encoding) {
   }
 };
 
-scale.domain = function (scaleDef, encoding, stats, opt) {
-  opt = opt || {};
-
+scale.domain = function (scaleDef, encoding, stats, facet) {
   var name = scaleDef.name;
 
   var encDef = encoding.encDef(name);
@@ -74,12 +70,13 @@ scale.domain = function (scaleDef, encoding, stats, opt) {
   }
 
   // For stack, use STACKED data.
-  if (name == opt.stack) {
+  var stack = encoding.stack();
+  if (stack && name === stack.value) {
     return {
       data: STACKED,
       field: encoding.fieldRef(name, {
         // If faceted, scale is determined by the max of sum in each facet.
-        prefn: (opt.facet ? 'max_' : '') + 'sum_'
+        prefn: (facet ? 'max_' : '') + 'sum_'
       })
     };
   }
diff --git a/src/compiler/stack.js b/src/compiler/stack.js
index 8b012fa42..eac521f0c 100644
--- a/src/compiler/stack.js
+++ b/src/compiler/stack.js
@@ -2,60 +2,13 @@
 
 require('../globals');
 
-var  marks = require('./marks');
-
 module.exports = stacking;
 
-function stacking(data, encoding, mdef) {
-  // check if the encoding supports stack
-  if (!marks[encoding.marktype()].stack) return false;
-
-  // TODO: add || encoding.has(LOD) here once LOD is implemented
-  if (!encoding.has(COLOR)) return false;
-
-  var dim = null, val = null, idx = null,
-    isXMeasure = encoding.isMeasure(X),
-    isYMeasure = encoding.isMeasure(Y),
-    facets = encoding.facets();
-
-  if (isXMeasure && !isYMeasure) {
-    dim = Y;
-    val = X;
-    idx = 0;
-  } else if (isYMeasure && !isXMeasure) {
-    dim = X;
-    val = Y;
-    idx = 1;
-  } else {
-    return null; // no stack encoding
-  }
-
-  // add transform to compute sums for scale
-  var stacked = {
-    name: STACKED,
-    source: encoding.dataTable(),
-    transform: [{
-      type: 'aggregate',
-      groupby: [encoding.fieldRef(dim)].concat(facets), // dim and other facets
-      summarize: [{ops: ['sum'], field: encoding.fieldRef(val)}]
-    }]
-  };
-
-  if (facets && facets.length > 0) {
-    stacked.transform.push({ //calculate max for each facet
-      type: 'aggregate',
-      groupby: facets,
-      summarize: [{
-        ops: ['max'],
-        // we want max of sum from above transform
-        field: encoding.fieldRef(val, {prefn: 'sum_'})
-      }]
-    });
-  }
-
-  data.push(stacked);
+function stacking(encoding, mdef, stack) {
+  var dim = stack.dimension;
+  var val = stack.value;
 
-  var valName = encoding.encDef(val).name;
+  var valName = encoding.fieldRef(val);
   var startField = valName + '_start';
   var endField = valName + '_end';
 
diff --git a/src/compiler/subfacet.js b/src/compiler/subfacet.js
index e2de0c7ca..29c566c9e 100644
--- a/src/compiler/subfacet.js
+++ b/src/compiler/subfacet.js
@@ -4,7 +4,7 @@ require('../globals');
 
 module.exports = subfaceting;
 
-function subfaceting(group, mdef, details, stack, encoding) {
+function subfaceting(group, mdef, details, encoding) {
   var m = group.marks;
   var g = {
     name: 'subfacet',
@@ -27,6 +27,7 @@ function subfaceting(group, mdef, details, stack, encoding) {
   trans.push({type: 'facet', groupby: details});
 
   // TODO: understand why we need this sort transform and write comment
+  var stack = encoding.stack();
   if (stack && encoding.has(COLOR)) {
     trans.unshift({type: 'sort', by: encoding.fieldRef(COLOR)});
   }


Test patch:
diff --git a/test/compiler/scale.spec.js b/test/compiler/scale.spec.js
index 3d67aa421..8229ac69b 100644
--- a/test/compiler/scale.spec.js
+++ b/test/compiler/scale.spec.js
@@ -27,34 +27,35 @@ describe('vl.compile.scale', function() {
     describe('for stack', function() {
       it('should return correct stack', function() {
         var domain = vlscale.domain(linearScaleDef, Encoding.fromSpec({
+          marktype: 'bar',
           encoding: {
             y: {
+              aggregate: 'sum',
               name: 'origin'
-            }
+            },
+            x: {name: 'x', type: 'O'},
+            color: {name: 'color', type: 'O'}
           }
-        }), {}, {
-          stack: 'y',
-          facet: true
-        });
+        }), {}, true);
 
         expect(domain).to.eql({
           data: 'stacked',
-          field: 'max_sum_origin'
+          field: 'max_sum_sum_origin'
         });
       });
 
       it('should return correct aggregated stack', function() {
         var domain = vlscale.domain(linearScaleDef, Encoding.fromSpec({
+          marktype: 'bar',
           encoding: {
             y: {
               aggregate: 'sum',
               name: 'origin'
-            }
+            },
+            x: {name: 'x', type: 'O'},
+            color: {name: 'color', type: 'O'}
           }
-        }), {}, {
-          stack: 'y',
-          facet: true
-        });
+        }), {}, true);
 
         expect(domain).to.eql({
           data: 'stacked',