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

repeat operator with defined width and height does not accept layer #8523

Closed
mattijn opened this issue Oct 29, 2022 · 7 comments · Fixed by #8723
Closed

repeat operator with defined width and height does not accept layer #8523

mattijn opened this issue Oct 29, 2022 · 7 comments · Fixed by #8723
Labels
Altair Issue that is blocking Altair Bug 🐛

Comments

@mattijn
Copy link
Contributor

mattijn commented Oct 29, 2022

This spec Open the Chart in the Vega Editor:

{
  "repeat": {
    "layer": ["US_Gross", "Worldwide_Gross"],
    "column": ["IMDB_Rating", "Rotten_Tomatoes_Rating"]
  },
  "spec": {
    "width": 200,
    "height": 130,
    "data": {
      "url": "https://cdn.jsdelivr.net/npm/[email protected]/data/movies.json"
    },
    "mark": "line",
    "encoding": {
      "color": {"datum": {"repeat": "layer"}},
      "x": {"bin": true, "field": {"repeat": "column"}},
      "y": {
        "aggregate": "mean",
        "field": {"repeat": "layer"},
        "title": "Mean of US and Worldwide Gross"
      }
    }
  },
  "$schema": "https://vega.github.io/schema/vega-lite/v5.2.0.json"
}

Renders correctly. But officially the layer cannot be defined, since the schema does not accept it:

image

Which is problematic for Altair-alike software

@mattijn mattijn changed the title repeat operator with layer and column does not accept width and height repeat operator with defined width and height does not accept layer Nov 15, 2022
@binste
Copy link

binste commented Jan 23, 2023

As you mentioned in the title, when you remove height and width from your example then the schema error disappears. In the vl schema, I think a repeat chart is either defined as non-layer or with layer:

"RepeatSpec": {
  "anyOf": [
    {
      "$ref": "#/definitions/NonLayerRepeatSpec"
    },
    {
      "$ref": "#/definitions/LayerRepeatSpec"
    }
  ]
}

I'm wondering if this example somehow does not fit both definitions and then shows the error based on matching the spec against "NonLayerRepeatSpec" where the error then is that "layer" is not allowed. The following warning suggests this as well:

[Warning] Validation: /repeat must NOT have additional properties of #/definitions/RepeatMapping/additionalProperties

-> Here it should not match against RepeatMapping which is referenced in NonLayerRepeatSpec but instead against LayerRepeatMapping.

Would you know any way in the Vega Editor (or other tool) to learn more about the warnings? It would be great to know why it doesn't match the layered one. I'll try to validate it in Python.

@binste
Copy link

binste commented Jan 23, 2023

I used python-jsonschema to validate the specs against NonLayerRepeatSpec and LayerRepeatSpec. Validating against NonLayerRepeatSpec gives me the same error as in the Vega Editor. When validating against LayerRepeatSpec, it has 2 options for "spec":

"spec": {
  "anyOf": [
    {
      "$ref": "#/definitions/LayerSpec"
    },
    {
      "$ref": "#/definitions/UnitSpec"
    }
  ],
  "description": "A specification of the view that gets repeated."
},

Validating the specs against LayerSpec fails anyway as that one does not has mark as an allowed property and layer is a required property. Validating against UnitSpec works but only if no width and height are specified as they are not allowed properties.

I think this means that there is currently no schema-conform way to specify height and width for a layered repeat chart. I don't know enough about Vega-Lite to solve this but I hope this gives someone else a good start to adjust the schema if necessary.

Appendix

For reference, code I used to validate the spec in Python:

import json

import altair as alt
import jsonschema
import jsonschema.validators
import jsonschema.exceptions

def create_validator(schema, rootschema=None):
    if rootschema is not None:
        validator_cls = jsonschema.validators.validator_for(rootschema)
        resolver = jsonschema.RefResolver.from_schema(rootschema)
    else:
        validator_cls = jsonschema.validators.validator_for(schema)
        # No resolver is necessary if the schema is already the full schema
        resolver = None

    validator_kwargs = {"resolver": resolver}
    if hasattr(validator_cls, "FORMAT_CHECKER"):
        validator_kwargs["format_checker"] = validator_cls.FORMAT_CHECKER
    validator = validator_cls(schema, **validator_kwargs)
    return validator

spec_json = """{
  "repeat": {
    "layer": ["US_Gross", "Worldwide_Gross"],
    "column": ["IMDB_Rating", "Rotten_Tomatoes_Rating"]
  },
  "spec": {
    "width": 200,
    "height": 130,
    "data": {
      "url": "https://cdn.jsdelivr.net/npm/[email protected]/data/movies.json"
    },
    "mark": "line",
    "encoding": {
      "color": {"datum": {"repeat": "layer"}},
      "x": {"bin": true, "field": {"repeat": "column"}},
      "y": {
        "aggregate": "mean",
        "field": {"repeat": "layer"},
        "title": "Mean of US and Worldwide Gross"
      }
    }
  }
}"""
spec = json.loads(spec_json)
layered_schema = {"$ref": "#/definitions/LayerRepeatSpec"}
# non_layered_schema = {"$ref": "#/definitions/NonLayerRepeatSpec"}
validator = create_validator(layered_schema, alt.Chart()._rootschema)
errors = list(validator.iter_errors(spec))

Travers the error hierarchy as I described in vega/altair#2842. Access .absolute_schema_path to see where in the schema the error appeared.

@ChristopherDavisUCI
Copy link
Contributor

ChristopherDavisUCI commented Jan 26, 2023

This is super helpful @binste! One thing I noticed is that if you use import altair.vegalite.v4 as alt (so that you're using an older Vega-Lite schema), then no errors show up.

I have an idea of trying to use UnitSpecWithFrame<Field> instead of (or in addition to) UnitSpec<Field>, but so far I haven't gotten it to work.

Edit: This is the change I tried to make: next...ChristopherDavisUCI:vega-lite:cd/allow-size My understanding is that running yarn build should generate a new version of the schema, but I'm still getting warnings, the first of which is Validation: must have required property 'data' of #/required

@binste
Copy link

binste commented Jan 28, 2023

Interesting! Just checked the v4 schema and indeed, in there the UnitSpec schema still allowed width and height. In v5 UnitSpec now refers to "GenericUnitSpec<Encoding,AnyMark>" which does not allow for those parameters. Maybe this was unintentional when changing the vega-lite schema?

Thank you @ChristopherDavisUCI for trying to rebuild the schema! I can't dedicate enough time right now to get up and running myself with vega-lite but really appreciate that you dig into this :) Hopefully a vega-lite contributor can jump in as well and help out.

@ChristopherDavisUCI
Copy link
Contributor

ChristopherDavisUCI commented Jan 28, 2023

Thanks @binste! I had another look and think I was able to find one possible solution to this issue in #8676. The main difference is allowing UnitSpecWithFrame to the current possibilities for the spec parameter. If you have any comments on this approach, I'm very happy to hear them! This is the resulting schema: https://raw.githubusercontent.com/ChristopherDavisUCI/vega-lite/cd/allow-size/build/vega-lite-schema.json

@mattijn Would it make sense to delete the $schema parameter from your code and from the linked Vega-Lite Editor spec in #8523 (comment)? My proposed solution updates the schema, so it will still not work with v5.2.0.

@mattijn
Copy link
Contributor Author

mattijn commented Jan 30, 2023

Nope, the $schema key only defines which version is used to validate against. It does not defines the VL-version that is used to render the spec.
If no $schema key is defined, the spec is not validated.

Rendering issues are still tracked by error and warning messages.

You'll have to build Vega-Editor linked to your updated Vega-Lite bundle plus new VL-schema.

I think it is a quite complicated proces before we can test and play with your updated vl-schema. I'm not sure how and if this can be simplified.

Maybe the editor can have a menu-setting where manually another VL-bundle/schema can be specified.

@ChristopherDavisUCI
Copy link
Contributor

Hi @mattijn, thanks for looking at it! I am able to get your spec in #8523 (comment) to work locally on my computer, but only if I change v5.2.0 to v5 (or remove the $schema property entirely). The layer property is still underlined if I keep v5.2.0.

Here is what it looks like locally with v5.json:

Screenshot 2023-01-30 at 11 41 07 AM

Here is what it looks like with v5.2.0.json:

Screenshot 2023-01-30 at 11 44 04 AM

@domoritz domoritz added the Altair Issue that is blocking Altair label Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Altair Issue that is blocking Altair Bug 🐛
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants