Repository: altair-viz/altair
Pull request #2823 (2023-01-09 08:39:34)

Expand mark spec when using to_dict
Suggestion to close https://github.com/altair-viz/altair/issues/2508. To test that the correct chart was created, (e.g. in a student assignment), it would be quite useful it if was possible to test that the functionality of the chart was the same, even if a slightly different syntax was used to create it. 

Currently, if I want to test that the correct mark was used in a spec where a mark option might or might not be present, I need to write tests something like this:

```py
chart = alt.Chart().mark_bar(color='blue')

chart_dict = chart.to_dict()
if isinstance(chart_dict['mark'], dict):
    # If the mark takes a param, such as `color='blue'` in this example
    assert chart_dict['mark']['type'] == 'bar'
else:
    # If the spec is just `chart = alt.Chart().mark_bar()`
    assert chart_dict['mark'] == 'bar'
```

With this PR, I can always write tests like this, which is more convenient:

```py
assert chart.to_dict()['mark']['type'] == 'bar'
```

This build upon #2813 in spirit, so that `to_dict()` will always return the "longer version" of a spec, expanding both shorthands to proper VegaLite encodings and with this PR also expanding single mark strings to a dictionary. A notable difference is that the shorthands are altair specific so we have to expand them, whether this expansion of the mark to a dictionary is not technically needed since vega-lite accepts a string too. This means that `to_dict()` can be used to test the functionality of the chart, whereas `chart.mark` etc can be used to check if the desired altair syntax was used  (e.g. if you want to check that a shorthand was used instead of the long form encoding specification)

joelostblom: I think this would happen for any test where the input syntax is not controlled, but we want to check the functionality of the generated output spec. For me that has been using the autograder Otter, but I don't see how other autograders would avoid this situation. 

While this PR would break a situation where someone has been relying on a test like:

```py
assert chart.to_dict()['mark'] == 'bar'
```

I would argue that this test is not ideal because it is checking the syntax used to create the chart rather than the functionality of the generated spec. With this PR, we create two path so that someone who wants to check the syntax used to generate a spec can query the chart object directly and someone who wants to query the functionality of the generated spec can rely on the `to_dict` method.

To me, this seems worthwhile, but curious to hear what others think.
mattijn: Thanks @joelostblom and @binste! The PR is well described and contains clear tests. I support the intention and it is also nice if one want to continue working on the specification in Vega-Lite. No need to expand `"mark": "circle"` into `"mark": {"type": "circle"}` anymore to add additional mark properties.

Patch:
diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py
index 0c8f9b27..270a6c6d 100644
--- a/altair/utils/schemapi.py
+++ b/altair/utils/schemapi.py
@@ -406,6 +406,8 @@ class SchemaBase(object):
             kwds = {
                 k: v for k, v in kwds.items() if k not in list(ignore) + ["shorthand"]
             }
+            if "mark" in kwds and isinstance(kwds["mark"], str):
+                kwds["mark"] = {"type": kwds["mark"]}
             result = _todict(
                 kwds,
                 context=context,
diff --git a/doc/user_guide/internals.rst b/doc/user_guide/internals.rst
index f63479ad..d7187db4 100644
--- a/doc/user_guide/internals.rst
+++ b/doc/user_guide/internals.rst
@@ -136,7 +136,7 @@ from the Vega-Lite documentation, which has the following JSON specification::
           {"a": "G","b": 19}, {"a": "H","b": 87}, {"a": "I","b": 52}
         ]
       },
-      "mark": "bar",
+      "mark": {"type": "bar"},
       "encoding": {
         "x": {"field": "a", "type": "ordinal"},
         "y": {"field": "b", "type": "quantitative"}
@@ -161,7 +161,7 @@ construct an Altair chart object from this string of Vega-Lite JSON:
           {"a": "G","b": 19}, {"a": "H","b": 87}, {"a": "I","b": 52}
         ]
       },
-      "mark": "bar",
+      "mark": {"type": "bar"},
       "encoding": {
         "x": {"field": "a", "type": "ordinal"},
         "y": {"field": "b", "type": "quantitative"}
@@ -186,7 +186,7 @@ you can use the :meth:`~Chart.from_dict` method to construct the chart object:
           {"a": "G","b": 19}, {"a": "H","b": 87}, {"a": "I","b": 52}
         ]
       },
-      "mark": "bar",
+      "mark": {"type": "bar"},
       "encoding": {
         "x": {"field": "a", "type": "ordinal"},
         "y": {"field": "b", "type": "quantitative"}
diff --git a/doc/user_guide/saving_charts.rst b/doc/user_guide/saving_charts.rst
index ec0d5939..6fc18526 100644
--- a/doc/user_guide/saving_charts.rst
+++ b/doc/user_guide/saving_charts.rst
@@ -61,7 +61,7 @@ The contents of the resulting file will look something like this:
           "type": "quantitative"
         }
       },
-      "mark": "point"
+      "mark": {"type": "point"}
     }
 
 This JSON can then be inserted into any web page using the vegaEmbed_ library.
@@ -121,7 +121,7 @@ javascript-enabled web browser:
               "type": "quantitative"
             }
           },
-          "mark": "point"
+          "mark": {"type": "point"}
         };
         var opt = {"renderer": "canvas", "actions": false};
         vegaEmbed("#vis", spec, opt);
diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py
index 190446bb..0ceb17b5 100644
--- a/tools/schemapi/schemapi.py
+++ b/tools/schemapi/schemapi.py
@@ -404,6 +404,8 @@ class SchemaBase(object):
             kwds = {
                 k: v for k, v in kwds.items() if k not in list(ignore) + ["shorthand"]
             }
+            if "mark" in kwds and isinstance(kwds["mark"], str):
+                kwds["mark"] = {"type": kwds["mark"]}
             result = _todict(
                 kwds,
                 context=context,


Test patch:
diff --git a/tests/tests/test_magics.py b/tests/tests/test_magics.py
index d794c733..db24ff48 100644
--- a/tests/tests/test_magics.py
+++ b/tests/tests/test_magics.py
@@ -45,7 +45,7 @@ VEGALITE_SPEC = {
         "x": {"field": "category", "type": "ordinal"},
         "y": {"field": "amount", "type": "quantitative"},
     },
-    "mark": "bar",
+    "mark": {"type": "bar"},
 }
 
 
diff --git a/tests/utils/tests/test_html.py b/tests/utils/tests/test_html.py
index 5f44704b..ccd41745 100644
--- a/tests/utils/tests/test_html.py
+++ b/tests/utils/tests/test_html.py
@@ -7,7 +7,7 @@ from altair.utils.html import spec_to_html
 def spec():
     return {
         "data": {"url": "data.json"},
-        "mark": "point",
+        "mark": {"type": "point"},
         "encoding": {
             "x": {"field": "x", "type": "quantitative"},
             "y": {"field": "y", "type": "quantitative"},
diff --git a/tests/utils/tests/test_mimebundle.py b/tests/utils/tests/test_mimebundle.py
index 6cdb9c37..3aa8734f 100644
--- a/tests/utils/tests/test_mimebundle.py
+++ b/tests/utils/tests/test_mimebundle.py
@@ -32,7 +32,7 @@ def vegalite_spec():
                 {"a": "I", "b": 52},
             ]
         },
-        "mark": "bar",
+        "mark": {"type": "bar"},
         "encoding": {
             "x": {"field": "a", "type": "ordinal"},
             "y": {"field": "b", "type": "quantitative"},
diff --git a/tests/utils/tests/test_schemapi.py b/tests/utils/tests/test_schemapi.py
index d7263404..f63ad29d 100644
--- a/tests/utils/tests/test_schemapi.py
+++ b/tests/utils/tests/test_schemapi.py
@@ -554,3 +554,11 @@ def test_to_dict_no_side_effects():
     assert "shorthand" not in dct["encoding"]["y"]
     assert dct["encoding"]["y"]["field"] == "b"
     assert dct["encoding"]["y"]["type"] == "quantitative"
+
+
+def test_to_dict_expand_mark_spec():
+    # Test that `to_dict` correctly expands marks to a dictionary
+    # without impacting the original spec which remains a string
+    chart = alt.Chart().mark_bar()
+    assert chart.to_dict()["mark"] == {"type": "bar"}
+    assert chart.mark == "bar"
diff --git a/tests/vegalite/tests/test_common.py b/tests/vegalite/tests/test_common.py
index 3a3bc4db..0cc216e6 100644
--- a/tests/vegalite/tests/test_common.py
+++ b/tests/vegalite/tests/test_common.py
@@ -11,7 +11,7 @@ from altair.vegalite import v5
 def basic_spec():
     return {
         "data": {"url": "data.csv"},
-        "mark": "line",
+        "mark": {"type": "line"},
         "encoding": {
             "color": {"type": "nominal", "field": "color"},
             "x": {"type": "quantitative", "field": "xval"},
diff --git a/tools/schemapi/tests/test_schemapi.py b/tools/schemapi/tests/test_schemapi.py
index 3dd93e4e..82c04fbb 100644
--- a/tools/schemapi/tests/test_schemapi.py
+++ b/tools/schemapi/tests/test_schemapi.py
@@ -552,3 +552,11 @@ def test_to_dict_no_side_effects():
     assert "shorthand" not in dct["encoding"]["y"]
     assert dct["encoding"]["y"]["field"] == "b"
     assert dct["encoding"]["y"]["type"] == "quantitative"
+
+
+def test_to_dict_expand_mark_spec():
+    # Test that `to_dict` correctly expands marks to a dictionary
+    # without impacting the original spec which remains a string
+    chart = alt.Chart().mark_bar()
+    assert chart.to_dict()["mark"] == {"type": "bar"}
+    assert chart.mark == "bar"