Repository: altair-viz/altair
Pull request #1521 (2019-05-20 23:51:49)

ENH: move layer & concat chart data to the top level when possible.
Addresses one part of #1500.

The enhancement here is that now you can do something like this:
```python
base = alt.Chart(df).encode(
    x='x', 
    y='y'
)
alt.layer(
    base.mark_point(),
    base.mark_line()
).facet(
    'category'
)
```
Previously, you had to do
```python
base = alt.Chart().encode(
    x='x', 
    y='y'
)
alt.layer(
    base.mark_point(),
    base.mark_line()
).facet(
    'category',
    data=df
)
```
or else the ``facet()`` would lead to an obscure error about missing data. With this change, both these options work correctly.

jakevdp: Note one consequence is that there is now a slight difference between ``a + b + c`` and ``alt.layer(a, b, c)``. In particular. If ``a`` and ``b`` both specify the same data and ``c`` specifies no data, then the former will move data to the top level, and the latter will not. I anticipate this being very confusing for any user that hits it, but I also think it will be a very rare situation. Unfortunately, I can't think of any way to change the behavior that doesn't introduce corner cases that are even worse.

Patch:
diff --git a/CHANGES.md b/CHANGES.md
index c2b7487a..6e9c9041 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -4,6 +4,13 @@
 
 ### Enhancements
 
+- ``data`` argument is now properly handled by  ``Chart.properties`` (#1525)
+
+- Compound charts (layer, concat, hconcat, vconcat) now move data to the top
+  level by default. In particular, this means that the ``facet()`` method
+  can now be called directly on a layered chart without having to change
+  how data is specified. (#1521)
+
 - ``alt.LayerChart`` now supports ``mark_*()`` methods. If a layer specifies a
   mark at the top level, all child charts will inherit it (unless they override
   it explicitly).
diff --git a/altair/vegalite/v3/api.py b/altair/vegalite/v3/api.py
index 84cd23b7..3cd0a918 100644
--- a/altair/vegalite/v3/api.py
+++ b/altair/vegalite/v3/api.py
@@ -477,15 +477,20 @@ class TopLevelMixin(mixins.ConfigMethodMixin):
         return result
 
     # Layering and stacking
-
     def __add__(self, other):
-        return LayerChart(layer=[self, other])
+        if not isinstance(other, TopLevelMixin):
+            raise ValueError("Only Chart objects can be layered.")
+        return layer(self, other)
 
     def __and__(self, other):
-        return VConcatChart(vconcat=[self, other])
+        if not isinstance(other, TopLevelMixin):
+            raise ValueError("Only Chart objects can be concatenated.")
+        return vconcat(self, other)
 
     def __or__(self, other):
-        return HConcatChart(hconcat=[self, other])
+        if not isinstance(other, TopLevelMixin):
+            raise ValueError("Only Chart objects can be concatenated.")
+        return hconcat(self, other)
 
     def repeat(self, repeat=Undefined, row=Undefined, column=Undefined, columns=Undefined, **kwargs):
         """Return a RepeatChart built from the chart
@@ -559,7 +564,7 @@ class TopLevelMixin(mixins.ConfigMethodMixin):
                 coefficient=Undefined, distance=Undefined, fraction=Undefined, lobes=Undefined,
                 parallel=Undefined, precision=Undefined, radius=Undefined, ratio=Undefined,
                 rotate=Undefined, spacing=Undefined, tilt=Undefined, **kwds):
-        """Add a geographic projection to the chartself.
+        """Add a geographic projection to the chart.
 
         This is generally used either with ``mark_geoshape`` or with the
         ``latitude``/``longitude`` encodings.
@@ -1705,10 +1710,12 @@ class ConcatChart(TopLevelMixin, core.TopLevelConcatSpec):
             _check_if_valid_subspec(spec, 'ConcatChart')
         super(ConcatChart, self).__init__(data=data, concat=list(concat),
                                           columns=columns, **kwargs)
+        self.data, self.concat = _combine_subchart_data(self.data, self.concat)
 
     def __ior__(self, other):
         _check_if_valid_subspec(other, 'ConcatChart')
         self.concat.append(other)
+        self.data, self.concat = _combine_subchart_data(self.data, self.concat)
         return self
 
     def __or__(self, other):
@@ -1730,10 +1737,12 @@ class HConcatChart(TopLevelMixin, core.TopLevelHConcatSpec):
         for spec in hconcat:
             _check_if_valid_subspec(spec, 'HConcatChart')
         super(HConcatChart, self).__init__(data=data, hconcat=list(hconcat), **kwargs)
+        self.data, self.hconcat = _combine_subchart_data(self.data, self.hconcat)
 
     def __ior__(self, other):
         _check_if_valid_subspec(other, 'HConcatChart')
         self.hconcat.append(other)
+        self.data, self.hconcat = _combine_subchart_data(self.data, self.hconcat)
         return self
 
     def __or__(self, other):
@@ -1755,10 +1764,12 @@ class VConcatChart(TopLevelMixin, core.TopLevelVConcatSpec):
         for spec in vconcat:
             _check_if_valid_subspec(spec, 'VConcatChart')
         super(VConcatChart, self).__init__(data=data, vconcat=list(vconcat), **kwargs)
+        self.data, self.vconcat = _combine_subchart_data(self.data, self.vconcat)
 
     def __iand__(self, other):
         _check_if_valid_subspec(other, 'VConcatChart')
         self.vconcat.append(other)
+        self.data, self.vconcat = _combine_subchart_data(self.data, self.vconcat)
         return self
 
     def __and__(self, other):
@@ -1782,10 +1793,12 @@ class LayerChart(TopLevelMixin, EncodingMixin, mixins.MarkMethodMixin,
         for spec in layer:
             _check_if_valid_subspec(spec, 'LayerChart')
         super(LayerChart, self).__init__(data=data, layer=list(layer), **kwargs)
+        self.data, self.layer = _combine_subchart_data(self.data, self.layer)
 
     def __iadd__(self, other):
         _check_if_valid_subspec(other, 'LayerChart')
         self.layer.append(other)
+        self.data, self.layer = _combine_subchart_data(self.data, self.layer)
         return self
 
     def __add__(self, other):
@@ -1881,3 +1894,29 @@ def topo_feature(url, feature, **kwargs):
     """
     return core.UrlData(url=url, format=core.TopoDataFormat(type='topojson',
                                                          feature=feature, **kwargs))
+
+
+def _combine_subchart_data(data, subcharts):
+    def remove_data(subchart):
+        if subchart.data is not Undefined:
+            subchart = subchart.copy()
+            subchart.data = Undefined
+        return subchart
+
+    if not subcharts:
+        # No subcharts = nothing to do.
+        pass
+    elif data is Undefined:
+        # Top level has no data; all subchart data must
+        # be identical to proceed.
+        subdata = subcharts[0].data
+        if subdata is not Undefined and all(c.data is subdata for c in subcharts):
+            data = subdata
+            subcharts = [remove_data(c) for c in subcharts]
+    else:
+        # Top level has data; subchart data must be either
+        # undefined or identical to proceed.
+        if all(c.data is Undefined or c.data is data for c in subcharts):
+            subcharts = [remove_data(c) for c in subcharts]
+
+    return data, subcharts
diff --git a/doc/user_guide/compound_charts.rst b/doc/user_guide/compound_charts.rst
index 4e73cfa7..e3c878ac 100644
--- a/doc/user_guide/compound_charts.rst
+++ b/doc/user_guide/compound_charts.rst
@@ -343,7 +343,7 @@ layered chart with a hover selection:
 
     hover = alt.selection_single(on='mouseover', nearest=True, empty='none')
 
-    base = alt.Chart().encode(
+    base = alt.Chart(iris).encode(
         x='petalLength:Q',
         y='petalWidth:Q',
         color=alt.condition(hover, 'species:N', alt.value('lightgray'))
@@ -352,100 +352,19 @@ layered chart with a hover selection:
         height=180,
     )
 
-    chart = base.mark_point().add_selection(
+    points = base.mark_point().add_selection(
         hover
     )
 
-    chart += base.mark_text(dy=-5).encode(
+    text = base.mark_text(dy=-5).encode(
         text = 'species:N',
         opacity = alt.condition(hover, alt.value(1), alt.value(0))
     )
 
-    chart.facet(
-        column='species:N',
-        data=iris
+    alt.layer(points, text).facet(
+        'species:N',
     )
 
-Notice that we specify the data within the facet here; this is important, because
-the top-level facet needs access to this data in order to know how to encode
-the column.
-
 Though each of the above examples have faceted the data across columns,
 faceting across rows (or across rows *and* columns) is supported as
-well.
-
-
-.. _compound-charts-data:
-
-Advanced: Compound Charts, Data, and Transforms
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-When using compound charts, it is possible to specify data in multiple places,
-and if you are using large datasets it can be pertinent to think about where
-you specify data.
-
-For example, suppose you are creating a layered plot with a dataset marked by
-both points and lines:
-
-.. altair-plot::
-    :chart-var-name: chart
-
-    import altair as alt
-    import pandas as pd
-
-    data = pd.DataFrame({'x': range(10)})
-
-    base = alt.Chart(data).transform_calculate(
-        y='sin(datum.x)'
-    ).encode(
-        x='x:Q',
-	y='y:Q'
-    )
-
-    chart = base.mark_point() + base.mark_line()
-
-If you examine the JSON specification generated by this chart, you see that
-both the data reference and the transform are specified twice, once per layer.
-
-.. altair-plot::
-   :output: stdout
-
-   from pprint import pprint
-   pprint(chart.to_dict())
-
-This happens because each layer of the chart is constructed from a copy of
-the ``base`` object we created, which contains its own data and transform.
-
-In the compound charts created by methods discussed in this section, sub-charts
-are aware of the data and transforms defined by their parent objects, and so
-you can create cleaner specifications by defining them just once at the
-top level. For layered charts, even the encodings can be specified at the
-top level:
-
-.. altair-plot::
-    :chart-var-name: chart
-		     
-    chart = alt.LayerChart(data).transform_calculate(
-        y='sin(datum.x)'
-    ).encode(
-        x='x:Q',
-        y='y:Q'
-    )
-
-    chart += alt.Chart().mark_point()
-    chart += alt.Chart().mark_line()
-
-When the chart is created this way, the spec contains only one copy of the
-data, transform, and encodings, which leads to a more efficient chart
-representation:
-
-.. altair-plot::
-   :output: stdout
-
-   pprint(chart.to_dict())
-
-Comparing this to the identically-rendered specification above, we see that
-there is much less duplication of information.
-
-This should not make any significant difference in chart behavior or
-performance, but for the advanced user it is useful to keep these
-features in mind.
+well.
\ No newline at end of file


Test patch:
diff --git a/altair/vegalite/v3/tests/test_api.py b/altair/vegalite/v3/tests/test_api.py
index 426a8555..9dd20ee4 100644
--- a/altair/vegalite/v3/tests/test_api.py
+++ b/altair/vegalite/v3/tests/test_api.py
@@ -2,6 +2,7 @@
 
 import io
 import json
+import operator
 import os
 import tempfile
 
@@ -18,6 +19,13 @@ except ImportError:
     selenium = None
 
 
+OP_DICT = {
+    'layer': operator.add,
+    'hconcat': operator.or_,
+    'vconcat': operator.and_,
+}
+
+
 @pytest.fixture
 def basic_chart():
     data = pd.DataFrame({
@@ -540,7 +548,10 @@ def test_chart_from_dict():
 
 
 def test_consolidate_datasets(basic_chart):
-    chart = basic_chart | basic_chart
+    subchart1 = basic_chart
+    subchart2 = basic_chart.copy()
+    subchart2.data = basic_chart.data.copy()
+    chart = subchart1 | subchart2
 
     with alt.data_transformers.enable(consolidate_datasets=True):
         dct_consolidated = chart.to_dict()
@@ -647,4 +658,53 @@ def test_data_property():
     chart1 = alt.Chart(data).mark_point()
     chart2 = alt.Chart().mark_point().properties(data=data)
 
-    assert chart1.to_dict() == chart2.to_dict()
\ No newline at end of file
+    assert chart1.to_dict() == chart2.to_dict()
+
+    
+@pytest.mark.parametrize('method', ['layer', 'hconcat', 'vconcat', 'concat'])
+@pytest.mark.parametrize('data', ['data.json', pd.DataFrame({'x': range(3), 'y': list('abc')})])
+def test_subcharts_with_same_data(method, data):
+    func = getattr(alt, method)
+
+    point = alt.Chart(data).mark_point().encode(x='x:Q', y='y:Q')
+    line = point.mark_line()
+    text = point.mark_text()
+
+    chart1 = func(point, line, text)
+    assert chart1.data is not alt.Undefined
+    assert all(c.data is alt.Undefined for c in getattr(chart1, method))
+
+    if method != 'concat':
+        op = OP_DICT[method]
+        chart2 = op(op(point, line), text)
+        assert chart2.data is not alt.Undefined
+        assert all(c.data is alt.Undefined for c in getattr(chart2, method))
+
+
+@pytest.mark.parametrize('method', ['layer', 'hconcat', 'vconcat', 'concat'])
+@pytest.mark.parametrize('data', ['data.json', pd.DataFrame({'x': range(3), 'y': list('abc')})])
+def test_subcharts_different_data(method, data):
+    func = getattr(alt, method)
+
+    point = alt.Chart(data).mark_point().encode(x='x:Q', y='y:Q')
+    otherdata = alt.Chart('data.csv').mark_point().encode(x='x:Q', y='y:Q')
+    nodata = alt.Chart().mark_point().encode(x='x:Q', y='y:Q')
+
+    chart1 = func(point, otherdata)
+    assert chart1.data is alt.Undefined
+    assert getattr(chart1, method)[0].data is data
+
+    chart2 = func(point, nodata)
+    assert chart2.data is alt.Undefined
+    assert getattr(chart2, method)[0].data is data
+
+
+def test_layer_facet(basic_chart):
+    chart = (basic_chart + basic_chart).facet(row='row:Q')
+    assert chart.data is not alt.Undefined
+    assert chart.spec.data is alt.Undefined
+    for layer in chart.spec.layer:
+        assert layer.data is alt.Undefined
+
+    dct = chart.to_dict()
+    assert 'data' in dct