Add Stack Transform #3771

invokesus · 2018-05-18T19:13:23Z

To be done:

~~Documentation~~
Examples
- Basic Example
- Diverging Stacked Bar
- Mosaic / Marimekko Chart
Test coverage
Let's add @hide to the description of StackTransform to hide it from the schema for now (and re-compile schema) so we can merge this after you address the unaddressed comments.
For examples you wanna add, please add _future suffix to the example files so they can be included even though stack transform wouldn't be in the schema yet.

kanitw · 2018-05-18T20:26:20Z

src/compile/data/parse.ts

+    } else if (isStack(t)) {
+      const stack = head = StackNode.makeFromTransform(head, t);
+
+      for (const field of keys(stack.producedFields())) {


This code is called repeatedly through this method. In a separate PR, we should refactor this part #3772.

kanitw

This is a great start. Please see comments below.

I appreciate that you try to make atomic commits overall!

Some tips:

Try to structure so that "refactor existing code" (like "Refactor to generate as property during make.") come earlier.
Try to avoid make later commits overriding prior changes. (Otherwise these commits are no longer atomic, and then reviewers need to look at more diff than they need to.)

Note: Github currently has a stupid bug that "hide some of my comments". Please make sure to expand each of them and address them accordingly.

kanitw · 2018-05-18T20:27:36Z

src/compile/data/stack.ts

+  /**
+   * Stack measure's field. Used in makeFromTransform
+   */
+  stack?: string;


Why is this different from field? Isn't it exactly the same thing?

Note that the we should design the component such that it represents information that's equilvalent to what we need in the output Vega stack transform. (We should later decouple things that we need for impute to a separate ImputeNode)

Which one should I keep stack or field?

We can also name it stackField.

(We later do rename it in the assemble method anyway):

const {facetby, field: stackField, dimensionFieldDef, impute, offset, sort, stackby} = this._stack;

kanitw · 2018-05-18T20:28:41Z

src/compile/data/stack.ts

-    out[this._stack.field + '_start'] = true;
-    out[this._stack.field + '_end'] = true;
-
+    const out = this._stack.as.reduce((result, item) => {


just return this without declaring?

kanitw · 2018-05-18T20:32:34Z

src/compile/data/stack.ts

   */
-  sort: VgSort;
+  sort: VgSort | SortField; // TODO Simplify the sort. make uses VgSort.


Why union type here? Why not converting SortField to VgSort right away?

kanitw · 2018-05-18T20:32:44Z

src/compile/data/stack.ts

   */
-  field: string;
+  field?: string;


Why is this optional?

Will keep either stack or field and make that compulsory.

kanitw · 2018-05-18T20:36:39Z

src/compile/data/stack.ts

-      const dimensionField = dimensionFieldDef ? vgField(dimensionFieldDef, {binSuffix: 'mid'}): undefined;
+    // Detects whether assemble call is from Node created from make or MakeFromTransform
+    // TODO Create a stricter function to check that.
+    if (this._stack.stackby) {


Let's not add this additional nesting level. See the rationale in the comment on the "else" line.

kanitw · 2018-05-18T22:47:23Z

src/transform.ts

+   */
+  groupby: string[];
+  /**
+   * Mode for stacking marks. Default is 'zero'


Use our standard default value format.

Mode for stacking marks. __Default value:__ `"zero"`

kanitw · 2018-05-18T22:50:27Z

src/transform.ts

+  sort?: SortField ;
+  /**
+   * Output field names.
+   * y2 or x2 is optional if only one name is provided the end will be “<name>_end”


People might not get what is x2 and y2. Maybe:

Output field names. This can be either a string or an array of strings with two elements denoting the name for the fields for stack start and stack end respectively.
If a single string is provided, the end field will be "_end".

If a single string is provided, the end field will be "_end".

Right now if a single string(eg. "values") is provided, the end field is "values_end".

kanitw · 2018-05-18T22:51:13Z

test/compile/data/stack.test.ts

-        "color": {"field": "c", "type": "ordinal",}
-      }
-    });
+  describe('StackNode.make', () => {


makeForEncoding?

kanitw · 2018-05-18T22:57:07Z

src/compile/data/stack.ts

+    } else {
+      const {stack, groupby, offset, sort, as} = this._stack;
+      let normalizedAs: Array<string>;
+      if (isAsValidArray(as)) {


Why is this logic for as appearing here too? I don't think this is necessary.

I just see that you removed this in a later commit.

kanitw · 2018-05-18T22:57:44Z

src/compile/data/stack.ts

@@ -23,7 +23,7 @@ function getStackByFields(model: UnitModel): string[] {

 export interface StackComponent {
  // TODO it would be cleaner to compose the two interfaces to create this one.
-  // TODO remove Used in *comments
+  // TODO Rename make to makeFromTransform


Rename make to makeFromEncoding?

…akFromEncoding

…ackTransform.

kanitw

Have a few more comments :)

(For examples, you might want to separate examples to another branch after addressing these few comments as we're still debating about it on Slack.)

kanitw · 2018-05-20T00:19:05Z

src/compile/data/stack.ts

+        value: 0
+      });
+    }
+    if(facetby) {


Why adding this if(facetby) {?

I'm not sure about this since I haven't read how facetby is generated, but the general logic behind the if is that there are 3 code segments in assemble:

Segment 1: when this._stack has been created by makeFromEncoding where impute: true

Segment 2: when this._stack has been created by makeFromEncoding

Segment 3. when this._stack has been created by makeFromTransform

The existing code structure led me to believe that Segment 1 and Segment 2 are not disjoint and both can be executed for a single input spec.

To maintain a flat structure, initially this._stack with impute:true is handled, then all the this._stack created by makeFromEncoding(including the ones operated on in the previous step), then finally the ones created by makeFromTransform which is disjoint to Segment 1 and 2.

facetby is undefined for any this._stack created by makeFromTransform . So those cases skip the if(facetby) { block.

kanitw · 2018-05-20T00:20:27Z

src/transform.ts

+   * Output field names. This can be either a string or an array of strings with
+   * two elements denoting the name for the fields for stack start and stack end
+   * respectively.
+   * If a single string(eg."val") is provided, the end field will be "val_end".


string(eg."val")

string (e.g., "val")

kanitw · 2018-05-20T00:22:29Z

src/compile/data/stack.ts

  /**
-   * Faceted field.
+   * Faceted field. Used in makeFromEncoding.


See 6bbd2d6#r189408463

kanitw · 2018-05-20T00:23:45Z

src/compile/data/stack.ts

+}
+// TODO Better Name for this function
+function isAsValidArray(as: string[] | string): as is string[] {
+  return isArray(as) && as.every(s => isString(s)) && as.length ===2;


If you really wanna keep this method, check if array.length > 1. (The 3rd, 4th, ... elements can be ignored.)

kanitw · 2018-05-20T00:24:12Z

src/transform.ts

+  /**
+   * Field that determines the order of leaves in the stacked charts.
+   */
+  sort?: SortField ;


Actually shouldn't it be SortField[] like window?

kanitw · 2018-05-20T00:25:12Z

src/compile/data/stack.ts

+  public static makeFromTransform(parent: DataFlowNode, stackTransform: StackTransform) {
+
+    const {stack, groupby, as, offset='zero'} = stackTransform;
+    const sort = stackTransform.sort || {'field': stack, 'order': 'ascending'};


I'm curious why this default is needed?

kanitw · 2018-05-20T01:40:45Z

Note that Github collapses a few of my comments again ><" -- make sure to look at them :)

kanitw · 2018-05-21T16:45:18Z

Let's add @hide to the description of StackTransform to hide it from the schema for now (and re-compile schema) so we can merge this after you address the unaddressed comments.
For examples you wanna add, please add _future suffix to the example files so they can be included even though stack transform wouldn't be in the schema yet.

kanitw · 2018-05-22T17:41:25Z

src/compile/data/stack.ts

+
+}
+
+function isAsValidArray(as: string[] | string): as is string[] {


isValidAsArray

kanitw · 2018-05-22T17:54:52Z

src/compile/data/stack.ts

        groupby: stackby,
        key: dimensionField,
        method: 'value',
        value: 0
      });
    }
+    if(facetby) {


In general, you should look at your overall PR diff and think why you do certain things.
Here you add if (facetby) to wrap the main code for the transform assemble logic (as it creates the stack transform). Having a case that we can skip the main logic is quite questionable.

From your earlier rationale, you say that facetby is undefined in makeFromTransform. But why should that be undefined? (That's actually something you set yourself.)

FYI, previously facetby was required but somehow you make it optional in this PR. (Why?)
I actually mentioned earlier that the facetby should be an empty array [] in either case.
Thus, facetby shouldn't be optional and then you don't have to add the if here.

Just FYI, it's okay to forget some details I mentioned because this is quite a complicated code base. But feel free to ask again if you are unsure about something. :)

A good case to test whether this logic is now correct for facetby is to try to see if you can modify
https://github.com/vega/vega-lite/blob/master/examples/specs/normalized/trellis_stacked_bar_normalized.vl.json
such that you have the stack transform inside the inner spec.

{ "$schema": "https://vega.github.io/schema/vega-lite/v2.json", "data": { "url": "data/barley.json" }, "facet": { "column": { "field": "year", "type": "ordinal" } }, "spec": { "transform": [{aggregate: ...}, {stack: ...}], "mark": "bar", "encoding": { ... } } }

Thanks for the review, Ham. I think now I understand where I was wrong. I made facetby a required field, and refactored assemble.
I must have missed your comment about trying out stack inside a faceted spec. It's working now.

kanitw · 2018-05-22T17:56:47Z

src/compile/data/stack.ts

+    let normalizedAs: Array<string>;
+    if (isAsValidArray(as)) {
+      normalizedAs = as;
+    } else if(typeof as === 'string') {


if(isString(as))

kanitw · 2018-05-22T20:37:29Z

src/compile/data/stack.ts

@@ -152,15 +207,14 @@ export class StackNode extends DataFlowNode {
      }
      return [vgField(dimensionFieldDef)];
    }
-    return [];
+    return groupby || [];


I'm ok with this for now.

Once you decouple Impute from stacking (#1514), we should make dimensionFieldDef no longer a part of StackComponent and just have StackNode.makeFromEncoding() create a groupby, using this method. At that point, we can basically remove facetBy and just have push new dimension in the facet to groupby instead.

(Basically, if you look at the code, the final groupby in the stack transform = groupBy that's from dimension fieldDef + facetBy)

kanitw · 2018-05-22T23:16:32Z

Since the code looks pretty stable at this point, I'll merge to a feature/stack branch for now. Please create another PR to merge to the feature branch.

* Add Stack Transform (#3771) * Add Stack Transform Examples (#3798) * Add Stacked population, Diverging Bar Chart example, Marimekko Chart of cars.json * Add 3 Mosaic plots of cars.json * Add compiled vega specs and svgs of the added examples * Transpose axes of rect_mosaic_simple to maintain consistency

invokesus added 6 commits May 19, 2018 00:04

Changed StackComponent to hold both Stack operations

cdd5773

Add StackTransform interface

38ea65c

Add StackTransform

091d309

Refactor to generate as property during make.

6171fe1

Added call to ancestorParse

d1de3e8

Add tests for StackTransform

d60db8e

invokesus requested a review from kanitw May 18, 2018 19:13

kanitw reviewed May 18, 2018

View reviewed changes

kanitw requested changes May 18, 2018

View reviewed changes

vega deleted a comment from invokesus May 18, 2018

invokesus added 7 commits May 19, 2018 13:35

Rename model-> stackTransform, field & stack -> stackField, make -> m…

bde6bdb

…akFromEncoding

Replace Union(VgSort,SortField) -> VgSort

6bbd2d6

Return producedFields without creating local variable

4e2c547

Call to StackNode.make -> StackNode.makeFromEncoding

e4dc989

Make "as" compulsory field in StackTransform, Docstring Changes in St…

89d411a

…ackTransform.

Change tests to adhere to new type signature of StackComponent

1283354

Add New Spec

7fc947a

kanitw requested changes May 20, 2018

View reviewed changes

invokesus added 4 commits May 22, 2018 22:45

Refactor to make StackTransform.sort of type SortField[]

92a3ebf

Change valid as field check, minor change to docstrings

65b927a

Refactor tests to pass for sort type SortField[]

6cf9b82

New Schema with changes to sort field type

8c87a04

kanitw reviewed May 22, 2018

View reviewed changes

Make assemble control flow linear

9fd5c68

kanitw reviewed May 22, 2018

View reviewed changes

kanitw mentioned this pull request May 22, 2018

Decouple ImputeNode from StackNode and perhaps Add Impute Transform #1514

Open

Hide stack transform from the schema

fe39dfa

kanitw changed the base branch from master to feature/stack May 22, 2018 20:58

kanitw merged commit 797ed94 into vega:feature/stack May 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Stack Transform #3771

Add Stack Transform #3771

invokesus commented May 18, 2018 •

edited by kanitw

Loading

kanitw May 18, 2018

kanitw left a comment •

edited

Loading

kanitw May 18, 2018 •

edited

Loading

invokesus May 19, 2018

kanitw May 19, 2018

kanitw May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018

invokesus May 19, 2018

kanitw May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018 •

edited

Loading

invokesus May 19, 2018 •

edited

Loading

kanitw May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018

kanitw left a comment

kanitw May 20, 2018

invokesus May 22, 2018

kanitw May 20, 2018

kanitw May 20, 2018

kanitw May 20, 2018

kanitw May 20, 2018

kanitw May 21, 2018

kanitw May 20, 2018

kanitw commented May 20, 2018

kanitw commented May 21, 2018 •

edited

Loading

kanitw May 22, 2018

kanitw May 22, 2018

invokesus May 22, 2018

kanitw May 22, 2018

kanitw May 22, 2018 •

edited

Loading

kanitw commented May 22, 2018


		}

		function isAsValidArray(as: string[] \| string): as is string[] {

Add Stack Transform #3771

Add Stack Transform #3771

Conversation

invokesus commented May 18, 2018 • edited by kanitw Loading

Choose a reason for hiding this comment

kanitw left a comment • edited Loading

Choose a reason for hiding this comment

kanitw May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw May 18, 2018 • edited Loading

Choose a reason for hiding this comment

invokesus May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw commented May 20, 2018

kanitw commented May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw May 22, 2018 • edited Loading

Choose a reason for hiding this comment

kanitw commented May 22, 2018

invokesus commented May 18, 2018 •

edited by kanitw

Loading

kanitw left a comment •

edited

Loading

kanitw May 18, 2018 •

edited

Loading

kanitw May 18, 2018 •

edited

Loading

invokesus May 19, 2018 •

edited

Loading

kanitw commented May 21, 2018 •

edited

Loading

kanitw May 22, 2018 •

edited

Loading