feat: default Vega-Lite-based renderer (#342) #438

peter-gy · 2022-11-01T17:22:58Z

Goals

Create a default renderer based on Vega-Lite
- Used Altair to get automatic VL schema validation
- The renderer returns an alt.VConcatChart | alt.HConcatChart | alt.FacetChart | alt.Chart, ready to be displayed, exported, customized, etc.
Define the schema of the Draco specification language using Pydantic
- Using raw dicts one has to fight a lot with the type checkers
- Having typed access to the schema makes its traversal and processing more readable

closes #342
spawns #449
spawns #455

We can delete this before merging, but the nb should be useful for discussions

peter-gy · 2022-11-01T17:56:10Z

Vega-Lite vs. Draco spec

Below I list my takeaways in a nutshell from implementing all the example charts from the docs (https://dig.cmu.edu/draco2/facts/examples.html) in Vega-Lite using Altair.

VL mostly covers all our rendering needs, in most cases the mapping from our schema to the VL schema is straightforward ✓
VL has no explicit support for toggling between cartesian and polar coordinate systems
- However, we can use an arc mark & the encodings theta, radius to overcome this ✓
As far as I can tell, VL does not allow for nested faceting / repeating / concatenation. This is problematic since Draco can produce multi-view recommendations and each view might or might not be faceted 🚫

Questions regarding Draco's spec

What do we consider as a strictly valid spec? Defining this would be important so that the renderer method can validate the input before trying to convert it into VL. For example, the following spec (Pie Chart Example) does not define a field for its encoding on the y channel:

{'field': [{'name': 'temperature', 'type': 'number'},
           {'name': 'condition', 'type': 'string'}],
 'number_rows': 100,
 'view': [{'coordinates': 'polar',
           'mark': [{'encoding': [{'aggregate': 'count',
                                   'channel': 'y',
                                   'stack': 'zero'},
                                  {'channel': 'color', 'field': 'condition'}],
                     'type': 'bar'}],
           'scale': [{'channel': 'y', 'type': 'linear', 'zero': 'true'},
                     {'channel': 'color', 'type': 'categorical'}]}]}

While it is easy to infer that the temperature field should be used here, is the renderer expected to handle this inferring?

What do you think, would it make sense to define a Draco spec strictly? We could use Pydantic for this & the implementation would mean relatively little effort. It has an awesome DX when it comes to defining schemas (see https://pydantic-docs.helpmanual.io/usage/schema/) and we already depend on the package.

@JunranY, @Zehua-Zeng, @domoritz

do you know a workaround to concatenate faceted views in VL?
what do you think about introducing a Pydantic-based schema validation?
in general, what kind of development roadmap would you prefer for this feature? Should we...
- merge an MVP as soon as possible that is capable of rendering simpler recs (basically any non-multi-view rec in a cartesian polar system) and work on further refinements iteratively, or
- create a more elaborate issue from this handling all specs & create a rendering base-class to support multiple renderer implementations?

lgtm-com · 2022-11-08T08:05:19Z

This pull request introduces 1 alert when merging 932ed30 into 0bf09e5 - view on LGTM.com

new alerts:

1 for Wrong name for an argument in a call

peter-gy · 2022-11-08T16:40:32Z

Hey @domoritz! While the code I pushed is indeed not really LGTM (as it is partial work full of quick & dirty experimenting), I believe the LGTM.com report above is not right:

kw_only is a totally valid parameter name since Python 3.10. Is it possible that the Python version based on which LGTM.com checks our code is not in sync with the version we actually use? I did not update it in 06144c4, as I have no access to the job's config as far as I know.

Renders all the example charts

draco/renderer/altair_renderer.py

The previous version crashed for some cases including `Generic` types

maybe fixes failing CI job https://github.com/cmudig/draco2/actions/runs/3439166059/jobs/5736131475

draco/renderer/altair_renderer.py

…ault-renderer Needed to pull in CI fix

cover validators

Support on-demand concats

draco/renderer/altair_renderer.py

peter-gy · 2022-11-18T18:35:15Z

@domoritz could you please take a look at this PR?

domoritz · 2022-11-23T18:47:16Z

.github/workflows/codeql.yml

@@ -1,4 +1,4 @@
-name: "CodeQL"


Not sure why this is in the diff. Do you need to rebase or merge main?

domoritz · 2022-11-23T18:50:32Z

draco/renderer/altair_renderer.py

+    represented as an `Altair <https://altair-viz.github.io/>`_ chart object.
+    """
+
+    def __init__(self, concat_mode: Literal["hconcat", "vconcat"] | None = None):


I think it would be better to move the concat mode into the Draco spec itself. This would simplify and generalize the logic here.

draco/renderer/altair_renderer.py

domoritz · 2022-11-23T18:54:53Z

draco/renderer/altair_renderer.py

+            case "bar":
+                encodes_x_and_y = all([e.channel in ["x", "y"] for e in encodings])
+                if encodes_x_and_y:
+                    return chart.mark_arc(stroke="#ffffff") + chart.mark_text(


Why do we set the stroke here?

The mental notes I took when writing this piece of code was the following:

When rendering a bar chart in a polar coordinate system, we are essentially rendering a classical radial chart in a cartesian coordinate system, using theta and radius as the channels instead of the polar x and y channels.

TLDR: I added a stroke here so there is a visual separation between the slices, just as we would have gaps between bars in a bar chart.

Without Stroke

Open the Chart in the Vega Editor

With Stroke

Open the Chart in the Vega Editor

domoritz · 2022-11-23T18:56:42Z

draco/renderer/altair_renderer.py

+                    f"Unknown channel: {encoding.channel}"
+                )  # pragma: no cover
+
+    def __get_field_type(self, fields: list[Field], field_name: FieldName) -> str:


I don't think this logic is correct. We should look at the scale type instead. The data type could be number but the scale could be categorical or continuous.

Meeting notes:

Scale type provides a cleaner mapping, as it makes it possible to differentiate between different number types such as numerical, ordinal, and categorical.

domoritz · 2022-11-23T19:02:41Z

draco/types.py

+import pydantic.main as pydantic_main
+import pydantic.types as pydantic_types
+
+"""


As discussed, this file is Vega specific and should be moved into the Vega renderer.

peter-gy · 2022-12-07T13:07:23Z

The failing workflows are most likely related to actions/setup-python#162.

Attempts to fix "Explicit returns mixed with implicit (fall through) returns"

Fixes actions/setup-python#162

A spec has either top-level- or view-level scales or both.

Adds support for determining Vega-Lite field type based on a combination of the scale type and the field's raw data type

domoritz

Just one last comment.

domoritz · 2022-12-13T22:02:07Z

draco/renderer/altair/altair_renderer.py

-            "datetime": "temporal",
+            "linear": {
+                "number": "quantitative",
+                "string": "nominal",


string and boolean are not supported for linear, no?

domoritz · 2022-12-13T22:02:20Z

draco/renderer/altair/altair_renderer.py

+            },
+            "log": {
+                "number": "quantitative",
+                "string": "nominal",


same as above

peter-gy · 2022-12-13T22:58:09Z

@domoritz, I added the discussed changes in bd7ca3b

draco/renderer/altair/altair_renderer.py

…encoding`

peter-gy added the enhancement New feature or request label Nov 1, 2022

peter-gy changed the title ~~feat: default Vega-Lite-based renderer (#342)~~ feat: default Vega-Lite-based renderer Nov 1, 2022

peter-gy changed the title ~~feat: default Vega-Lite-based renderer~~ feat: default Vega-Lite-based renderer (#342) Nov 1, 2022

feat: create examples-vl notebook

3de1ba4

We can delete this before merging, but the nb should be useful for discussions

peter-gy force-pushed the feat/342-default-renderer branch from 575e07e to 3de1ba4 Compare November 1, 2022 17:25

feat: base renderer progress

932ed30

peter-gy and others added 3 commits November 8, 2022 14:34

refactor: extract renderer base

50e0dfd

ci: set up python before installing stuff

535dcb4

rm poetry

6f33063

feat: AltairRenderer stable impl

01e2eba

Renders all the example charts

github-advanced-security bot found potential problems Nov 10, 2022

View reviewed changes

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

peter-gy added 4 commits November 10, 2022 18:03

chore: update mypy

43644e6

The previous version crashed for some cases including `Generic` types

refactor: satisfy type analyzers

182c8d1

chore: merge main

705a68f

chore: bump wheel

68e65e8

maybe fixes failing CI job https://github.com/cmudig/draco2/actions/runs/3439166059/jobs/5736131475

github-advanced-security bot found potential problems Nov 10, 2022

View reviewed changes

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

peter-gy added 2 commits November 10, 2022 22:28

docs: document renderer

f15dda8

docs: document types

2630f61

github-advanced-security bot found potential problems Nov 10, 2022

View reviewed changes

draco/renderer/altair_renderer.py Fixed Show resolved Hide resolved

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

peter-gy added 2 commits November 10, 2022 22:52

chore: update poetry.lock

9f8162d

Merge remote-tracking branch 'origin/code-scanning' into feat/342-def…

01ec390

…ault-renderer Needed to pull in CI fix

peter-gy force-pushed the feat/342-default-renderer branch from 8ae9280 to e78f11a Compare November 10, 2022 22:05

chore: downgrade wheel

fc68609

peter-gy force-pushed the feat/342-default-renderer branch from e78f11a to fc68609 Compare November 10, 2022 22:21

peter-gy added 3 commits November 10, 2022 23:49

test: test types

2c9cac7

cover validators

refactor: remove unused utility method

1da11b6

test: increase renderer coverage

d7a789e

peter-gy mentioned this pull request Nov 15, 2022

feat: migrate to draco2 peter-gy/VisRecly#54

Open

peter-gy added 8 commits November 18, 2022 16:26

feat: validate polar marks in schema

bcaa8fe

test: polar mark validator

0925055

feat: validate polar encodings in schema

1d972e0

test: polar encoding validator

8910c7a

feat: support rendering polar charts

94dc9e9

test: polar charts

ea7e7a7

feat: handle multi views

bb7547a

Support on-demand concats

test: multi views

dfada10

github-advanced-security bot found potential problems Nov 18, 2022

View reviewed changes

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

draco/renderer/altair_renderer.py Fixed Show fixed Hide fixed

domoritz requested changes Nov 23, 2022

View reviewed changes

chore: merge main

6a36bad

fix: satisfy CodeQL

76795ec

Attempts to fix "Explicit returns mixed with implicit (fall through) returns"

peter-gy force-pushed the feat/342-default-renderer branch from 24a0a8d to 76795ec Compare December 7, 2022 14:11

peter-gy added 2 commits December 7, 2022 17:01

refactor: move vega-specific spec to altair package

d6f06a8

ci: pin runners to ubuntu-20.04

8b96f55

Fixes actions/setup-python#162

peter-gy mentioned this pull request Dec 7, 2022

Revert to using ubuntu-latest in CI/CD scripts #455

Closed

peter-gy added 4 commits December 7, 2022 23:24

docs: explain the purpose of the stroke in __visit_mark_polar

320cf54

refactor: declare methods as static where applicable

aac04b9

docs: document scale options of SpecificationDict

39e400a

A spec has either top-level- or view-level scales or both.

feat: consider scale type in __get_field_type

4e380ec

Adds support for determining Vega-Lite field type based on a combination of the scale type and the field's raw data type

domoritz approved these changes Dec 13, 2022

View reviewed changes

refactor: adjust dict in __get_field_type

bd7ca3b

domoritz approved these changes Dec 13, 2022

View reviewed changes

draco/renderer/altair/altair_renderer.py Outdated Show resolved Hide resolved

fix: distinguish __get_scale_for_encoding and `__get_alt_scale_for_…

25659fe

…encoding`

peter-gy merged commit dc03ecc into main Dec 14, 2022

peter-gy deleted the feat/342-default-renderer branch December 14, 2022 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: default Vega-Lite-based renderer (#342) #438

feat: default Vega-Lite-based renderer (#342) #438

peter-gy commented Nov 1, 2022 •

edited

peter-gy commented Nov 1, 2022

lgtm-com bot commented Nov 8, 2022

peter-gy commented Nov 8, 2022 •

edited

peter-gy commented Nov 18, 2022

domoritz Nov 23, 2022

domoritz Nov 23, 2022

domoritz Nov 23, 2022

peter-gy Dec 7, 2022 •

edited

domoritz Nov 23, 2022

peter-gy Dec 7, 2022

domoritz Nov 23, 2022

peter-gy commented Dec 7, 2022 •

edited

domoritz left a comment

domoritz Dec 13, 2022

domoritz Dec 13, 2022

peter-gy commented Dec 13, 2022

feat: default Vega-Lite-based renderer (#342) #438

feat: default Vega-Lite-based renderer (#342) #438

Conversation

peter-gy commented Nov 1, 2022 • edited

Goals

peter-gy commented Nov 1, 2022

Vega-Lite vs. Draco spec

Questions regarding Draco's spec

lgtm-com bot commented Nov 8, 2022

peter-gy commented Nov 8, 2022 • edited

peter-gy commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-gy Dec 7, 2022 • edited

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

peter-gy commented Dec 7, 2022 • edited

domoritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-gy commented Dec 13, 2022

peter-gy commented Nov 1, 2022 •

edited

peter-gy commented Nov 8, 2022 •

edited

peter-gy Dec 7, 2022 •

edited

peter-gy commented Dec 7, 2022 •

edited