Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Syntax for Wrapped/Flow Layout Support for Repeat / Facet / Concat #4457

Closed
kanitw opened this issue Jan 15, 2019 · 9 comments
Closed

Syntax for Wrapped/Flow Layout Support for Repeat / Facet / Concat #4457

kanitw opened this issue Jan 15, 2019 · 9 comments
Assignees
Labels
Area - View Composition P2 Important Issues that should be fixed soon

Comments

@kanitw
Copy link
Member

kanitw commented Jan 15, 2019

We previously discussed this in #393, but that discussion has been too long to follow (and sidetracked by comments about a slightly different discussion).

So I'd like to start a new issue to summarize proposed solutions.

We have considered two possible syntaxes for supporting wrappable facet/repeat concat so far

A) Using unnested facet / repeat / concat as the "general" facet / repeat / concat syntax that supports wrapping

Basically, a general 1D facet spec could look like this:

{
  // if facet is fieldDef right away, then it's a wrapped facet
  "facet": {"field": "Origin", "type": "nominal"},
  "columns": 4,  // The maximum number of columns on a row
  "spec": ...
}

then we can have the following syntax as our shorthand (just like row / column encoding channel).

{
  ...
  "encoding": {
    "facet": {"field": "Origin", "type": "nominal", "columns": 4}
  }
}

Note that we discussed the possibility of a {facet: {wrap: ...}, ...} syntax earlier. In that case, the macro's channel name could be "wrap". However, @jakevdp suggests that it could be confusing, especially in Altair. I agree with him.

We can do the same unnested repeat:

{
  // if repeat is an array of repeatet value rigth away, it's a wrapped repeat
  "repeat": [..., ..., ...],
  "columns": 4, 
  "spec": ...
}

This could work well in a consistent fashion with a wrapped concat.

{
  concat: [spec1, spec2, spec3, spec4],
  columns: 2
}

Note that I thought about the name for wrapped concat for a while, but wrappedConcat, flow, grid all sound worse than simply a generic concat, which can serve as a generic concat (vconcat when columns=1 and hconcat when columns = undefined / Infinity or wrapped concat for other numbers of columns).

By using the term concat, we can preserve grid for a possible macro/shorthand like this:

{
  grid: [
    [spec1, spec2], 
    [spec3, spec4]
  ]
}

(This grid thing has a lower priority though).

Pro

  • The syntax is quite concise. It does not require an additional level of nesting when we only facet / repeat by one field (rather than two). Faceting by one field is gonna be much more common use case, so opting for concision for this use case is favorable.

  • We can achieve a consistent syntax for generic facet/repeat/concat (which can wrap / or not depending on the number of columns).

Cons

  • The transition (spec edits) from faceting by one field to two fields is a bit more involved as users need to add one additional level of nesting. Same for repeating. (That said, I think this is less important than having a simple syntax for faceting by one field, which would cover the majority of the use case.)

B) Extend existing row/column/hconcat/vconcat syntax to support columns (or rows)

The other alternative is to extend existing row/column syntax:

{
  data: ...
  facet: {
    column: {field: 'category', type: 'nominal'}
  },
  columns: 5, // there will be 5 columns per row
  spec: ...
}
{
  data: ...
  facet: {
    row: {field: 'category', type: 'nominal'}
  },
  rows: 5, // there will be 5 rows per column
  spec: ...
}

If both facet.row and facet.column are specified, columns or rows will be ignored and there will no wrapping.

One minor note is that the latter (rows) requires some extension to Vega. (We could try to do the similar math in Vega-Lite, but I don't think we could as doing the same in Vega will benefit both abstraction level.)

We could do the same for repeat:

{
  repeat: {
    column: [..., ..., ...]
  },
  columns: 5, // there will be 5 columns per row
  spec: ...
}

and concat:

{
  hconcat: [..., ..., ...],
  columns: 5 // there will be 5 columns per row
}

Pros

  • This syntax is nice in the sense that we preserve the existing syntax and only need to extend the facet spec with rows/columns.

Cons

  • It's semantically misleading. A wrapped facet does not encode data by either row or column but rather both. hconcat with a number of columns is also misleading as it's no longer purely "horizontal". I think this is a critical con for this design.

  • It leads to a complex schema when both facet.row and facet.column are specified, columns or rows will be ignored and there will no wrapping.

  • (Minor point) Supporting rows requires some works. This solution requires that we support rows right away to make it not weird. (The first solution is fine with just columns for now.)


As I summarize these pros and cons for these two approaches, I think a) is a stronger solution.

cc: @domoritz @arvind @jheer @jakevdp -- Please comment if you have any thoughts.

@kanitw kanitw added this to the x.x Composition Patches milestone Jan 15, 2019
@kanitw kanitw changed the title Syntax Wrapped/Flow Layout Support for Repeat / Facet / Concat Syntax for Wrapped/Flow Layout Support for Repeat / Facet / Concat Jan 18, 2019
@kanitw kanitw self-assigned this Jan 19, 2019
@domoritz
Copy link
Member

I like a) a lot. It seems much more intuitive than b). I still struggle to understand when to use column or row and a) makes it less important.

Another future extension of either syntax could be to support faceting by multiple fields at once (multiple group by's). Not saying that we should do this soon but we are not preventing this extension.

{
  "facet": [
    {"field": "Origin", "type": "nominal"},
    {"field": "Cylinders", "type": "ordinal"},
  ]
  "columns": 4,  // The maximum number of columns on a row
  "spec": ...
}

@kanitw kanitw added the P2 Important Issues that should be fixed soon label Feb 1, 2019
@kanitw
Copy link
Member Author

kanitw commented Feb 10, 2019

One thing that I don't like about a) is that the repeater syntax will use "repeat" as both key and value:

x: {field: {repeat: "repeat"}, ...}

because now repeat is a repeat channel name just like row/column

x: {field: {repeat: "row/column"}, ...}

That said, I guess this is probably fine.

@kanitw
Copy link
Member Author

kanitw commented Feb 24, 2019

@jheer @arvind @domoritz Given that we will do a), I have a follow up question:

For the shorthand, should the composition layout properties be in fieldDef or in the outer spec?

a-1) in fieldDef

  • General Facet
{
  ...
  "encoding": {
    "facet": {
      "field": "Origin", 
      "type": "nominal", 
      "columns": 4,
      "spacing": 20, // number | {row: number, column: number}
      "center": ..., // boolean | {row: boolean, column: boolean}
      "align": ...,  // "all" | `"each" | "none"` | {row: ..., column: ...}
    }
  }
}
  • Row/Column Facet
{
  ...
  "encoding": {
    "row/column": {
      "field": "Origin", 
      "type": "nominal", 
      "spacing": 20 // this would be only either row/column spacing
      "center": ... // this would be only either row/column center
      "align": ...  // this would be only either row/column align
    } 
  }
}

a-2) outside

  • General Facet
{
  ...
  "columns": 4,
  "spacing": 20, // number | {row: number, column: number}
  "center": ..., // boolean | {row: boolean, column: boolean}
  "align": ...,  // "all" | `"each" | "none"` | {row: ..., column: ...}

  "mark": ...,
  "encoding": {
    "facet": {"field": "Origin", "type": "nominal"}
  }
}
  • Row/Column Facet
{
  ...
  "spacing": 20, // number | {row: number, column: number}
  "center": ..., // boolean | {row: boolean, column: boolean}
  "align": ...,  // "all" | `"each" | "none"` | {row: ..., column: ...}

  "mark": ...,
  "encoding": {
    "row/column": {"field": "Origin", "type": "nominal"}
  }
}

a-1) is good that the layout properties are specific so the properties are quite "localized" to facet channel. However, the facet field def will be quite more complicated.

Meanwhile, a-2) offers a more flexible setup and less complicated schema.
This also won't require any modification to the logic (we only need to modify typings, this logic comes for free based on our macro properties extraction logic.).

I'm leaning towards a-2) for the sake of simplicity. Plus, even if we add a-1), a-2) would still be available, which means we offer multiple ways of doing things.

@kanitw kanitw mentioned this issue Feb 24, 2019
6 tasks
@domoritz
Copy link
Member

a-2 seems cleaner to me as well since you need to add objects in the general facet case anyway and the nesting gets pretty deep.

@kanitw
Copy link
Member Author

kanitw commented Feb 24, 2019

Cool thanks -- This issue will be finished in #4541

@kanitw kanitw closed this as completed Feb 24, 2019
@domoritz
Copy link
Member

Just noting here that after a discussing with @davidanthoff we noted that these two are equivalent and so this can help resolve #393 (comment).

{
  "name": "trellis_barley",
  "data": {"url": "data/barley.json"},
  "columns": 2,
  "facet": {
    "field": "site",
    "type": "ordinal",
    "sort": {"op": "median", "field": "yield"}
  },
  "spec" : {
    "mark": "point",
    "encoding": {
      "x": {
        "aggregate": "median",
        "field": "yield",
        "type": "quantitative",
        "scale": {"zero": false}
      },
      "y": {
        "field": "variety",
        "type": "ordinal",
        "sort": {"encoding": "x", "order": "descending"},
        "scale": {"rangeStep": 12}
      },
      "color": {"field": "year", "type": "nominal"}
    }
  }
}
{
  "name": "trellis_barley",
  "data": {"url": "data/barley.json"},
  "mark": "point",
  "columns": 2,
  "encoding": {
    "facet": {
      "field": "site",
      "type": "ordinal",
      "sort": {"op": "median", "field": "yield"}
    },
    "x": {
      "aggregate": "median",
      "field": "yield",
      "type": "quantitative",
      "scale": {"zero": false}
    },
    "y": {
      "field": "variety",
      "type": "ordinal",
      "sort": {"encoding": "x", "order": "descending"},
      "scale": {"rangeStep": 12}
    },
    "color": {"field": "year", "type": "nominal"}
  }
}

@jheer
Copy link
Member

jheer commented Mar 25, 2019

Somehow I am only getting to this issue now. I apologize I missed it earlier!

The design arguments above strike me as rather system-developer-centric. It is my strong suspicion that users would find the localized a-1 examples easier to understand (I certainly do). In any case, I naturally assumed that facet wrap would place the parameters within the facet definition itself, not outside it. The outside placement confused me as it seems to unnecessarily disconnect logically connected things. (Or, does it make sense to define any of those properties outside specific composition operators?)

From the standpoint of user-centric API design I would argue for including support for a-1.

@jheer
Copy link
Member

jheer commented Mar 25, 2019

Unfortunately, the use of facet at both the spec-level and the encoding-level causes a potential namespace collision for external APIs that use Vega-Lite. For example, vl.facet(...) could mean either an encoding-level facet channel or a spec-level facet composition operator. Obviously it's too late to change this for v3, but this design choice has already led to errors/incompatibilities over in vega-lite-api. As a result, I will have to redesign parts of the API.

@kanitw
Copy link
Member Author

kanitw commented Mar 25, 2019

It is my strong suspicion that users would find the localized a-1 examples easier to understand (I certainly do). In any case, I naturally assumed that facet wrap would place the parameters within the facet definition itself, not outside it.

It's not too late to add support for a-1 and deprecate a-2 (with backward compatibility support).

The facet channel name for the macro is definitely harder to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - View Composition P2 Important Issues that should be fixed soon
Projects
None yet
Development

No branches or pull requests

3 participants