Layer #1241

domoritz · 2016-03-14T20:28:04Z

Need to get these working:

{
  "data": {"url": "data/stocks.csv","formatType": "csv"},
  "layers": [
    {
      "mark": "line",
      "encoding": {
        "x": {"field": "date","type": "temporal"},
        "y": {"field": "price","type": "quantitative"},
        "color": {"field": "symbol","type": "nominal"}
      }
    },
    {
      "mark": "rule",
      "encoding": {
        "x": {
          "field": "date",
          "type": "temporal",
          "aggregate": "mean"
        },
        "color": {"value": "red"}
      }
    }
  ]
}

{
  "data": {"url": "data/flights-2k.json"},
  "layers": [
    {
      "mark": "bar",
      "encoding": {
        "x": {
          "field": "distance",
          "type": "quantitative",
          "bin": true
        },
        "y": {
          "aggregate": "count",
          "field": "*",
          "type": "quantitative"
        }
      }
    },
    {
      "transform": {"filter": "datum.delay < 5"},
      "mark": "bar",
      "encoding": {
        "x": {
          "field": "distance",
          "type": "quantitative",
          "bin": true
        },
        "y": {
          "aggregate": "count",
          "field": "*",
          "type": "quantitative"
        },
        "color": {"value": "goldenrod"}
      }
    }
  ]
}

domoritz · 2016-03-16T23:07:59Z

My test spec

{
  "data": {
    "values": [
      {"a": "A","b": "B","c": 28,"d": "a"}, {"a": "B","b": "C","c": 55,"d": "a"},
      {"a": "C","b": "D","c": 43,"d": "a"}, {"a": "D","b": "E","c": 91,"d": "a"},
      {"a": "E","b": "F","c": 81,"d": "a"}, {"a": "F","b": "G","c": 53,"d": "b"},
      {"a": "G","b": "H","c": 19,"d": "b"}, {"a": "H","b": "I","c": 87,"d": "c"},
      {"a": "I","b": "A","c": 52,"d": "c"}
    ]
  },
  "layers": [
    {
      "mark": "bar",
      "encoding": {
        "x": {"field": "a","type": "ordinal"},
        "y": {"field": "c","type": "quantitative"},
        "color": {"field": "d", "type": "nominal"}
      }
    }, {
      "mark": "line",
      "encoding": {
        "x": {"field": "b","type": "ordinal"},
        "y": {"field": "c","type": "quantitative"},
        "color": {"value": "purple"}
      }
    }, {
      "mark": "rule",
      "encoding": {
        "y": {"field": "c", "type": "quantitative", "aggregate": "mean"},
        "color": {"field": "d", "type": "nominal"},
        "size": {"field": "c", "type": "quantitative", "aggregate": "max"}
      }
    }
  ]
}

Fixing sizing, config order

kanitw · 2016-03-17T02:30:18Z

src/compile/model.ts

+   * Function to rename the dataset name using the name map. Used in the assemble step.
+   */
+  public dataRename(name: string): string {
+    return this._dataNameMap.get(name);


This is get, not rename. I also have the renameData method right above on line 226.

kanitw · 2016-03-18T23:55:52Z

src/compile/data.ts

@@ -287,13 +298,11 @@ export namespace formatParse {
    // if there is no parse needed
    model.forEach(function(fieldDef: FieldDef) {
      if (fieldDef.type === TEMPORAL) {
-        parseComponent = parseComponent || {};


Have you thoroughly tested that removing this doesn't cause problem?

If you look above, you see that there is a let parseComponent: Dict<string> = {}; so this is fine.

Yep. But before this PR, a part of source assemble code assumes that formatParse component either contains something or is null. I'm not saying that this is worse, but need to make sure that this doesn't break those parts.

I fixed that part

…to the parent

@domoritz

…ecause now the value can be either `true` or `false`. This way, we can distinguish between when we only care about the key or when we care about the value too. (Although the underlying type of `StringSet` is also `Dict<Boolean>`) cc: @domoritz

kanitw · 2016-03-19T00:03:27Z

src/compile/data.ts

@@ -479,12 +506,13 @@ export namespace nullFilter {
  }

  export function parseLayer(model: LayerModel) {
+    // note that we run this before source.parseLayer
    let nullFilterComponent = parse(model);


We might not even need to call parse here because layers doesn't have its own field. This is always an empty object.

kanitw · 2016-03-19T00:04:31Z

src/compile/data.ts

    let nullFilterComponent = parse(model);

    model.children().forEach((child) => {
      const childDataComponent = child.component.data;
-      if (!childDataComponent.source && childDataComponent.nullFilter) {
-        extend(nullFilterComponent || {}, childDataComponent.nullFilter);
+      if (model.compatibleSource(child) && !differ(childDataComponent.nullFilter, nullFilterComponent)) {


If two children of the layer have the same nullFilterComponent they still don't get merge here.
Because each of the child won't be equal to the layer's initial nullFilterComponent which is simply {}.

domoritz · 2016-03-19T00:24:54Z

note to self: timeUnit should not be executed before source in data.ts

kanitw · 2016-03-19T20:24:47Z

src/compile/data.ts

+            mergeMeasures(summaries[key].measures, summary.measures);
+          } else {
+            // give the summary a new name
+            summary.name = model.dataName(SUMMARY) + '_' + keys(summaries).length;


keys(summaries).length

Should we name it by the dimensions included instead of a summary counter?

Good point. I changed the way summaries are generated and now it would make more sense to use the fields (+the source name?). But I don't want to worry about all the edge cases (explicit values, ...)

Ups, our key may be wrong in that case...

Not sure what "explicit values" mean in this case?

Ups, our key may be wrong in that case...

?? Can you elaborate?

summary_bin_distance_start_bin_distance_mid_bin_distance_end_bin_distance_range nope nope nope

Ok let's use number for now although a semantically meaningful mean could just be summary_bin_distance

maybe. I don't quite know what happens if we reference different datasets. I think we should forbid different datasets somehow!

I think forbidding simplify things for now, but not sure if we should forbid this in long run.

@domoritz

cc: @domoritz

kanitw · 2016-03-19T23:36:32Z

src/compile/data.ts

+  export function parseLayer(model: LayerModel) {
+    // note that we run this before source.parseLayer
+
+    // FIXME: null filters are not properly propagated right now


It's not only "not properly propagated". The merge logic below is still incorrect. I'll add this to #1254.

Layer

domoritz added 9 commits March 14, 2016 13:25

Remove isFacet, cellHeight and cellWidth from unit

97f96af

Ignore vscode

b9a259d

Use scaleType

0ad9bb8

Implement basic layering (assumes shared scales)

3e293d1

Basic layout for layering

70233c0

Support legends in layering

bccdedc

Clean up some of the data stuff

110affb

Simplify layout

a2b3b50

Fix issues with scales and data.

3b18e8e

Add rule mark

f85374f

domoritz force-pushed the layer branch from 3e397aa to f85374f Compare March 16, 2016 23:13

domoritz and others added 8 commits March 16, 2016 17:01

simplify path group name

82a5d34

Correctly merge layer data

89dfb99

Add vscode settings

45344db

Support size for rules

2d818d6

add lineSize, ruleSize, rename lineSizeRange to ruleSizeRange

b5c14b9

fix incorrect config merging order

cb56846

isFacet isLayer and isUnit

22add05

Merge pull request #1247 from vega/kw/layer

2d40fd5

Fixing sizing, config order

kanitw reviewed Mar 17, 2016
View reviewed changes

kanitw reviewed Mar 18, 2016
View reviewed changes

domoritz and others added 3 commits March 18, 2016 16:56

cc @kanitw I believe this is the right way to merge the summary up in…

492b3ac

…to the parent

Lint

fecb7bb

kanitw force-pushed the layer branch from 8208121 to 6d3130a Compare March 19, 2016 00:01

kanitw reviewed Mar 19, 2016
View reviewed changes

hamlint

78bc6a6

kanitw reviewed Mar 19, 2016
View reviewed changes

Order data component collection

42394ec

kanitw reviewed Mar 19, 2016
View reviewed changes

kanitw added 2 commits March 19, 2016 13:57

add more comments and minor refactor for 6343f56

9233fcf

cc: @domoritz

add missing property to VgScale interface

c4a13a6

kanitw force-pushed the layer branch from 223ef1c to c4a13a6 Compare March 19, 2016 20:57

kanitw and others added 4 commits March 19, 2016 15:55

replace reduce with forEach for assembleData and assembleLayout

88b26fb

//

c3025af

Typo

c536479

Better messages/ comments

5db41e4

domoritz force-pushed the layer branch from 00ce56d to 5db41e4 Compare March 19, 2016 23:20

kanitw reviewed Mar 19, 2016
View reviewed changes

More utils, improve hash function

8be45dd

kanitw added a commit that referenced this pull request Mar 20, 2016

Merge pull request #1241 from vega/layer

58b6f8b

Layer

kanitw merged commit 58b6f8b into master Mar 20, 2016

kanitw deleted the layer branch March 20, 2016 18:48

kanitw mentioned this pull request Mar 20, 2016

Layered Charts #585

Closed

domoritz mentioned this pull request Mar 29, 2016

Support multiple source datasets like vega #1271

Closed

TomBor mentioned this pull request Jan 25, 2017

support of "layers" specification (multiple marks in one chart) hrbrmstr/vegalite#16

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Layer #1241

Layer #1241

domoritz commented Mar 14, 2016

domoritz commented Mar 16, 2016

kanitw Mar 17, 2016

kanitw Mar 18, 2016

domoritz Mar 19, 2016

kanitw Mar 19, 2016

domoritz Mar 19, 2016

kanitw Mar 19, 2016

kanitw Mar 19, 2016

domoritz commented Mar 19, 2016

kanitw Mar 19, 2016

domoritz Mar 19, 2016

domoritz Mar 19, 2016

kanitw Mar 19, 2016

kanitw Mar 19, 2016

domoritz Mar 19, 2016

kanitw Mar 19, 2016

domoritz Mar 19, 2016

kanitw Mar 20, 2016

kanitw Mar 19, 2016

Layer #1241

Layer #1241

Conversation

domoritz commented Mar 14, 2016

domoritz commented Mar 16, 2016

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

domoritz commented Mar 19, 2016

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

Choose a reason for hiding this comment