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

Vega-Lite 4.1.1: some layered specs are invalid #5854

Closed
jakevdp opened this issue Feb 3, 2020 · 13 comments · Fixed by #5898
Closed

Vega-Lite 4.1.1: some layered specs are invalid #5854

jakevdp opened this issue Feb 3, 2020 · 13 comments · Fixed by #5898
Assignees
Labels
Bug 🐛 P1 Critical -- to fix ASAP

Comments

@jakevdp
Copy link
Contributor

jakevdp commented Feb 3, 2020

Several dozen of Altair's examples are invalid under the Vega-Lite 4.1.1 schema; the common feature seems to be that they contain layered specs. An example is the Bar and Tick Chart (view in vega editor).

Here is how I checked the validity under the schema; it results in a validation error under the v4.1.1 schema, but is fine under the v4.0.2 schema.

import jsonschema
import json
import urllib.request
with urllib.request.urlopen('https://vega.github.io/schema/vega-lite/v4.1.1.json') as f:
    schema = json.load(f)

spec = {
  "data": {
    "values": [
      {"goal": 25, "project": "a", "score": 25},
      {"goal": 47, "project": "b", "score": 57},
      {"goal": 30, "project": "c", "score": 23},
      {"goal": 27, "project": "d", "score": 19},
      {"goal": 38, "project": "e", "score": 8},
      {"goal": 19, "project": "f", "score": 47},
      {"goal": 4, "project": "g", "score": 8}
    ]
  },
  "layer": [
    {
      "encoding": {
        "x": {"field": "project", "type": "nominal"},
        "y": {"field": "score", "type": "quantitative"}
      },
      "mark": "bar",
      "width": {"step": 40}
    },
    {
      "encoding": {
        "x": {"field": "project", "type": "nominal"},
        "y": {"field": "goal", "type": "quantitative"}
      },
      "mark": {"color": "red", "size": 36, "thickness": 2, "type": "tick"}
    }
  ]
}

jsonschema.validate(spec, schema)
Full Traceback
---------------------------------------------------------------------------
ValidationError                           Traceback (most recent call last)
<ipython-input-12-c79b6a6118cf> in <module>
     36 }
     37 
---> 38 jsonschema.validate(spec, schema)

~/anaconda/lib/python3.6/site-packages/jsonschema/validators.py in validate(instance, schema, cls, *args, **kwargs)
    932     error = exceptions.best_match(validator.iter_errors(instance))
    933     if error is not None:
--> 934         raise error
    935 
    936 

ValidationError: Additional properties are not allowed ('mark' was unexpected)

Failed validating 'additionalProperties' in schema[0]:
    {'additionalProperties': False,
     'description': 'A full layered plot specification, which may contains '
                    '`encoding` and `projection` properties that will be '
                    'applied to underlying unit (single-view) '
                    'specifications.',
     'properties': {'data': {'anyOf': [{'$ref': '#/definitions/Data'},
                                       {'type': 'null'}],
                             'description': 'An object describing the data '
                                            'source. Set to `null` to '
                                            "ignore the parent's data "
                                            'source. If no data is set, it '
                                            'is derived from the parent.'},
                    'description': {'description': 'Description of this '
                                                   'mark for commenting '
                                                   'purpose.',
                                    'type': 'string'},
                    'encoding': {'$ref': '#/definitions/Encoding',
                                 'description': 'A shared key-value '
                                                'mapping between encoding '
                                                'channels and definition '
                                                'of fields in the '
                                                'underlying layers.'},
                    'height': {'anyOf': [{'type': 'number'},
                                         {'enum': ['container'],
                                          'type': 'string'},
                                         {'$ref': '#/definitions/Step'}],
                               'description': 'The height of a '
                                              'visualization.\n'
                                              '\n'
                                              '- For a plot with a '
                                              'continuous y-field, height '
                                              'should be a number.\n'
                                              '- For a plot with either a '
                                              'discrete y-field or no '
                                              'y-field, height can be '
                                              'either a number indicating '
                                              'a fixed height or an object '
                                              'in the form of `{step: '
                                              'number}` defining the '
                                              'height per discrete step. '
                                              '(No y-field is equivalent '
                                              'to having one discrete '
                                              'step.)\n'
                                              '- To enable responsive '
                                              'sizing on height, it should '
                                              'be set to `"container"`.\n'
                                              '\n'
                                              '__Default value:__ Based on '
                                              '`config.view.continuousHeight` '
                                              'for a plot with a '
                                              'continuous y-field and '
                                              '`config.view.discreteHeight` '
                                              'otherwise.\n'
                                              '\n'
                                              '__Note:__ For plots with '
                                              '[`row` and `column` '
                                              'channels](https://vega.github.io/vega-lite/docs/encoding.html#facet), '
                                              'this represents the height '
                                              'of a single view and the '
                                              '`"container"` option cannot '
                                              'be used.\n'
                                              '\n'
                                              '__See also:__ '
                                              '[`height`](https://vega.github.io/vega-lite/docs/size.html) '
                                              'documentation.'},
                    'layer': {'description': 'Layer or single view '
                                             'specifications to be '
                                             'layered.\n'
                                             '\n'
                                             '__Note__: Specifications '
                                             'inside `layer` cannot use '
                                             '`row` and `column` channels '
                                             'as layering facet '
                                             'specifications is not '
                                             'allowed. Instead, use the '
                                             '[facet '
                                             'operator](https://vega.github.io/vega-lite/docs/facet.html) '
                                             'and place a layer inside a '
                                             'facet.',
                              'items': {'anyOf': [{'$ref': '#/definitions/LayerSpec'},
                                                  {'$ref': '#/definitions/UnitSpec'}]},
                              'type': 'array'},
                    'name': {'description': 'Name of the visualization for '
                                            'later reference.',
                             'type': 'string'},
                    'projection': {'$ref': '#/definitions/Projection',
                                   'description': 'An object defining '
                                                  'properties of the '
                                                  'geographic projection '
                                                  'shared by underlying '
                                                  'layers.'},
                    'resolve': {'$ref': '#/definitions/Resolve',
                                'description': 'Scale, axis, and legend '
                                               'resolutions for view '
                                               'composition '
                                               'specifications.'},
                    'title': {'anyOf': [{'$ref': '#/definitions/Text'},
                                        {'$ref': '#/definitions/TitleParams'}],
                              'description': 'Title for the plot.'},
                    'transform': {'description': 'An array of data '
                                                 'transformations such as '
                                                 'filter and new field '
                                                 'calculation.',
                                  'items': {'$ref': '#/definitions/Transform'},
                                  'type': 'array'},
                    'view': {'$ref': '#/definitions/ViewBackground',
                             'description': 'An object defining the view '
                                            "background's fill and "
                                            'stroke.\n'
                                            '\n'
                                            '__Default value:__ none '
                                            '(transparent)'},
                    'width': {'anyOf': [{'type': 'number'},
                                        {'enum': ['container'],
                                         'type': 'string'},
                                        {'$ref': '#/definitions/Step'}],
                              'description': 'The width of a '
                                             'visualization.\n'
                                             '\n'
                                             '- For a plot with a '
                                             'continuous x-field, width '
                                             'should be a number.\n'
                                             '- For a plot with either a '
                                             'discrete x-field or no '
                                             'x-field, width can be either '
                                             'a number indicating a fixed '
                                             'width or an object in the '
                                             'form of `{step: number}` '
                                             'defining the width per '
                                             'discrete step. (No x-field '
                                             'is equivalent to having one '
                                             'discrete step.)\n'
                                             '- To enable responsive '
                                             'sizing on width, it should '
                                             'be set to `"container"`.\n'
                                             '\n'
                                             '__Default value:__\n'
                                             'Based on '
                                             '`config.view.continuousWidth` '
                                             'for a plot with a continuous '
                                             'x-field and '
                                             '`config.view.discreteWidth` '
                                             'otherwise.\n'
                                             '\n'
                                             '__Note:__ For plots with '
                                             '[`row` and `column` '
                                             'channels](https://vega.github.io/vega-lite/docs/encoding.html#facet), '
                                             'this represents the width of '
                                             'a single view and the '
                                             '`"container"` option cannot '
                                             'be used.\n'
                                             '\n'
                                             '__See also:__ '
                                             '[`width`](https://vega.github.io/vega-lite/docs/size.html) '
                                             'documentation.'}},
     'required': ['layer'],
     'type': 'object'}

On instance:
    {'encoding': {'x': {'field': 'project', 'type': 'nominal'},
                  'y': {'field': 'score', 'type': 'quantitative'}},
     'mark': 'bar',
     'width': {'step': 40}}
@jakevdp
Copy link
Contributor Author

jakevdp commented Feb 3, 2020

Digging-in a bit more: the common feature in every failure is that they specify a width or height within a layer. Here's a minimal failure case:

import jsonschema
import json
import urllib.request
with urllib.request.urlopen('https://vega.github.io/schema/vega-lite/v4.1.1.json') as f:
    schema = json.load(f)

spec = {
  "data": {"values": []},
  "layer": [
    {"mark": "point", "width": 100}
  ]
}

jsonschema.validate(spec, schema)

Removing "width": 100 causes the validation to succeed. Under the 4.0.2 schema, the spec is valid with the width.

@kanitw
Copy link
Member

kanitw commented Feb 4, 2020

This was actually intentional, because the layers do share width and thus specifying width inside means that the width could be conflicting.

That being said, I'm happy to revert if you disagree.

@kanitw kanitw added P3 Should be fixed at some point Need Clarification ❔ Needs clarification before we can proceed. and removed P3 Should be fixed at some point labels Feb 4, 2020
@jakevdp
Copy link
Contributor Author

jakevdp commented Feb 4, 2020

I see, that makes sense.

This is a fairly large-impact API breakage on Altair's side, though it may be that we could handle it with some changes on the Python side. For example, we could change the layering code such that when a user passes a chart with a width or height argument to a layer, those values get moved to the top level.

What do you think?

@kanitw
Copy link
Member

kanitw commented Feb 4, 2020

That makes sense. I'm happy to figure a different way to fix to avoid major breaking changes on the Altair side.

@jakevdp
Copy link
Contributor Author

jakevdp commented Feb 5, 2020

OK, I'll give that a shot tonight and see if it works.

@jakevdp
Copy link
Contributor Author

jakevdp commented Feb 5, 2020

Were there any other changes to allowed entries in unit specs?

@kanitw
Copy link
Member

kanitw commented Feb 6, 2020

Just width, height, and view (which couldn't be different between layers)

This was the PR for this change: #5797.

@domoritz
Copy link
Member

domoritz commented Feb 6, 2020

It's probably best to revert and show a warning if users specify conflicting properties. We could change what we allow at the unit spec level in the next major version.

@domoritz domoritz closed this as completed Feb 6, 2020
@domoritz domoritz reopened this Feb 6, 2020
@jakevdp
Copy link
Contributor Author

jakevdp commented Feb 12, 2020

Just to be clear, will this be reverted, or should I start adapting Altair with this behavior in mind? Altair is now 2 releases behind Vega-Lite because of this bug.

@domoritz
Copy link
Member

I'll revert it right now. We are making another release this week.

@domoritz
Copy link
Member

Fixed in #5891

@kanitw kanitw added P1 Critical -- to fix ASAP and removed Need Clarification ❔ Needs clarification before we can proceed. labels Feb 12, 2020
kanitw added a commit that referenced this issue Feb 12, 2020
kanitw added a commit that referenced this issue Feb 12, 2020
@jakevdp
Copy link
Contributor Author

jakevdp commented Feb 12, 2020

Thanks!

@kanitw
Copy link
Member

kanitw commented Feb 13, 2020

Sure, you're welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 P1 Critical -- to fix ASAP
Projects
None yet
3 participants