Repository: altair-viz/altair
Pull request #450 (2018-02-16 15:17:19)

API change: make chart methods return copies
With the stacking and layering available in VegaLite 2.0, I think it makes sense to have chart methods return copies of the chart rather than mutate the chart.

What this means is that with this PR in place, we could do things like this:

```python
chart = Chart(data).encode(x='x', y='y')
chart.mark_line() + chart.mark_point()
```
![vega 2](https://user-images.githubusercontent.com/781659/36314673-6caf5dc2-12ea-11e8-952f-5700a25661bc.png)
The result is a layered chart marked by both points and lines

similarly for faceting:
```python
chart = alt.Chart(data).mark_point().encode(y='y')
chart.encode(x='x1') | chart.encode(x='x2')
```
![vega 3](https://user-images.githubusercontent.com/781659/36314659-61621c16-12ea-11e8-888e-c8fa68b3d219.png)
The result is a two-panel side-by-side chart with a shared y-axis, but different encodings on the x axes. Previously this kind of layering and stacking would require much more boilerplate, or an explicit copy of the chart.

The pattern that will not work anymore after this change is code like this:
```python
chart = Chart(data)
chart.mark_line()
chart.encode(x='x', y='y')
```
Instead, you'd have to do
```python
chart = Chart(data)
chart = chart.mark_line()
chart = chart.encode(x='x', y='y')
```

But, to be clear, the standard chaining approach used in our examples will still work correctly after this change:

```python
Chart(data).mark_line().encode(
    x='x',
    y='y'
)
```

I think the advantages of this change far outweigh the disadvantages, but I wanted to get some second opinions before I push it. Any thoughts? cc @ellisonbg @craigcitro @domoritz @kanitw


Patch:
diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py
index e1a37cfa..7be60bd6 100644
--- a/altair/utils/schemapi.py
+++ b/altair/utils/schemapi.py
@@ -1,6 +1,6 @@
 # The contents of this file are automatically written by
 # tools/generate_schema_wrapper.py. Do not modify directly
-# 2018-02-15 13:27:12
+# 2018-02-18 21:30:42
 import collections
 import json
 
@@ -40,16 +40,37 @@ class SchemaBase(object):
         object.__setattr__(self, '_args', args)
         object.__setattr__(self, '_kwds', kwds)
 
-    def copy(self, deep=False):
-        """Return a deep copy of the object"""
-        args = self._args
-        kwds = self._kwds
+    def copy(self, deep=True, ignore=()):
+        """Return a copy of the object
 
+        Parameters
+        ----------
+        deep : boolean, optional
+            if True (default) then return a deep copy of all dict, list, and
+            SchemaBase objects within the object structure
+        ignore : list, optional
+            A list of keys for which the contents should not be copied, but
+            only stored by reference.
+        """
+        def _deep_copy(obj, ignore=()):
+            if isinstance(obj, SchemaBase):
+                args = tuple(_deep_copy(arg) for arg in obj._args)
+                kwds = {k: (_deep_copy(v, ignore=ignore)
+                            if k not in ignore else v)
+                        for k, v in obj._kwds.items()}
+                return obj.__class__(*args, **kwds)
+            elif isinstance(obj, list):
+                return [_deep_copy(v, ignore=ignore) for v in obj]
+            elif isinstance(obj, dict):
+                return {k: (_deep_copy(v, ignore=ignore)
+                            if k not in ignore else v)
+                        for k, v in obj.items()}
+            else:
+                return obj
         if deep:
-            import copy
-            args = copy.deepcopy(args)
-            kwds = copy.deepcopy(kwds)
-        return self.__class__(*args, **kwds)
+            return _deep_copy(self, ignore=ignore)
+        else:
+            return self.__class__(*self._args, **self._kwds)
 
     def __getattr__(self, item):
         # reminder: getattr is called after the normal lookups
@@ -122,7 +143,7 @@ class SchemaBase(object):
                               if k not in ignore})
         else:
             raise ValueError("{0} instance has both a value and properties : "
-                             "cannot serialize to dict")
+                             "cannot serialize to dict".format(self.__class__))
         if validate:
             self.validate(result)
         return result
diff --git a/altair/vegalite/v2/api.py b/altair/vegalite/v2/api.py
index 0baa1cd7..eabce410 100644
--- a/altair/vegalite/v2/api.py
+++ b/altair/vegalite/v2/api.py
@@ -68,80 +68,106 @@ class Chart(TopLevelMixin, core.TopLevelFacetedUnitSpec):
         super(Chart, self).__init__(data=data, encoding=encoding, mark=mark,
                                     width=width, height=height, **kwargs)
 
-    @use_signature(core.MarkConfig)
-    def mark_area(self, *args, **kwargs):
-        self.mark = 'area'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_bar(self, *args, **kwargs):
-        self.mark = 'bar'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_line(self, *args, **kwargs):
-        self.mark = 'line'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_point(self, *args, **kwargs):
-        self.mark = 'point'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_text(self, *args, **kwargs):
-        self.mark = 'text'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_tick(self, *args, **kwargs):
-        self.mark = 'tick'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_rect(self, *args, **kwargs):
-        self.mark = 'rect'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_rule(self, *args, **kwargs):
-        self.mark = 'rule'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_circle(self, *args, **kwargs):
-        self.mark = 'circle'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_square(self, *args, **kwargs):
-        self.mark = 'square'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    @use_signature(core.MarkConfig)
-    def mark_geoshape(self, *args, **kwargs):
-        self.mark = 'geoshape'
-        self.configure_mark(*args, **kwargs)
-        return self
-
-    def configure_mark(self, *args, **kwargs):
-        if args or kwargs:
-            if self.config is Undefined:
-                self.config = core.Config()
-            self.config.mark = core.MarkConfig(*args, **kwargs)
-        return self
-
-    # TODO: add hooks for more configure functions
+    @use_signature(core.MarkDef)
+    def mark_area(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='area', **kwargs)
+        else:
+            copy.mark = 'area'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_bar(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='bar', **kwargs)
+        else:
+            copy.mark = 'bar'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_line(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='line', **kwargs)
+        else:
+            copy.mark = 'line'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_point(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='point', **kwargs)
+        else:
+            copy.mark = 'point'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_text(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='text', **kwargs)
+        else:
+            copy.mark = 'text'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_tick(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='tick', **kwargs)
+        else:
+            copy.mark = 'tick'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_rect(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='rect', **kwargs)
+        else:
+            copy.mark = 'rect'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_rule(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='rule', **kwargs)
+        else:
+            copy.mark = 'rule'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_circle(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='circle', **kwargs)
+        else:
+            copy.mark = 'circle'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_square(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='square', **kwargs)
+        else:
+            copy.mark = 'square'
+        return copy
+
+    @use_signature(core.MarkDef)
+    def mark_geoshape(self, **kwargs):
+        copy = self.copy(deep=True, ignore=['data'])
+        if kwargs:
+            copy.mark = core.MarkDef(type='geoshape', **kwargs)
+        else:
+            copy.mark = 'geoshape'
+        return copy
+
+    # TODO: add configure_* methods
 
     def encode(self, *args, **kwargs):
         # First convert args to kwargs by inferring the class from the argument
@@ -163,9 +189,22 @@ class Chart(TopLevelMixin, core.TopLevelFacetedUnitSpec):
                 # Don't validate now, because field will be computed
                 # as part of the to_dict() call.
                 kwargs[prop] = cls.from_dict(field, validate=False)
-        # TODO: update nested values rather than overwriting them
-        self.encoding = core.EncodingWithFacet(**kwargs)
-        return self
+        copy = self.copy(deep=True, ignore=['data'])
+
+        # get a copy of the dict representation of the previous encoding
+        encoding = copy.encoding
+        if encoding is Undefined:
+            encoding = {}
+        elif isinstance(encoding, dict):
+            pass
+        else:
+            encoding = {k: v for k, v in encoding._kwds.items()
+                        if v is not Undefined}
+
+        # update with the new encodings, and apply them to the copy
+        encoding.update(kwargs)
+        copy.encoding = core.EncodingWithFacet(**encoding)
+        return copy
 
     def interactive(self, name='grid', bind_x=True, bind_y=True):
         """Make chart axes scales interactive
@@ -181,10 +220,12 @@ class Chart(TopLevelMixin, core.TopLevelFacetedUnitSpec):
             encodings.append('x')
         if bind_y:
             encodings.append('y')
-        self.selection = {name: {'bind': 'scales',
+        copy = self.copy(deep=True, ignore=['data'])
+        # TODO: don't overwrite previous selections?
+        copy.selection = {name: {'bind': 'scales',
                                  'type': 'interval',
                                  'encodings': encodings}}
-        return self
+        return copy
 
 
 class HConcatChart(TopLevelMixin, core.TopLevelHConcatSpec):
@@ -204,6 +245,7 @@ class VConcatChart(TopLevelMixin, core.TopLevelVConcatSpec):
 
     # TODO: think about the most useful class API here
 
+
 class LayerChart(TopLevelMixin, core.TopLevelLayerSpec):
     def __init__(self, layer, **kwargs):
         # TODO: move common data to top level?
diff --git a/altair/vegalite/v2/schema/channels.py b/altair/vegalite/v2/schema/channels.py
index 46af9be0..da7398fc 100644
--- a/altair/vegalite/v2/schema/channels.py
+++ b/altair/vegalite/v2/schema/channels.py
@@ -1,5 +1,5 @@
 # The contents of this file are automatically generated
-# 2018-02-15 13:27:12
+# 2018-02-18 21:30:42
 
 from . import core
 from altair.utils.schemapi import Undefined
diff --git a/altair/vegalite/v2/schema/core.py b/altair/vegalite/v2/schema/core.py
index aba84744..ee67250a 100644
--- a/altair/vegalite/v2/schema/core.py
+++ b/altair/vegalite/v2/schema/core.py
@@ -1,5 +1,5 @@
 # The contents of this file are automatically generated
-# at time 2018-02-15 13:27:12
+# at time 2018-02-18 21:30:42
 
 from altair.utils.schemapi import SchemaBase, Undefined
 
diff --git a/tools/schemapi/decorator.py b/tools/schemapi/decorator.py
index 5554b23e..d624c63d 100644
--- a/tools/schemapi/decorator.py
+++ b/tools/schemapi/decorator.py
@@ -34,17 +34,18 @@ def schemaclass(*args, init_func=True, docstring=True, property_map=True):
             warnings.warn("class is not an instance of SchemaBase.")
 
         name = cls.__name__
-        schema = SchemaInfo(cls)
 
         if init_func and '__init__' not in cls.__dict__:
-            init_code = codegen.init_code(name, schema.schema)
+            init_code = codegen.init_code(name, schema=cls._schema,
+                                          rootschema=cls._rootschema)
             globals_ = {name: cls, 'Undefined': Undefined}
             locals_ = {}
             exec(init_code, globals_, locals_)
             setattr(cls, '__init__', locals_['__init__'])
 
         if docstring and not cls.__doc__:
-            setattr(cls, '__doc__', codegen.docstring(name, schema.schema))
+            setattr(cls, '__doc__', codegen.docstring(name, schema=cls._schema,
+                                                      rootschema=cls._rootschema))
         return cls
 
     if len(args) == 0:
diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py
index 881ccf9e..ba4b7060 100644
--- a/tools/schemapi/schemapi.py
+++ b/tools/schemapi/schemapi.py
@@ -37,16 +37,37 @@ class SchemaBase(object):
         object.__setattr__(self, '_args', args)
         object.__setattr__(self, '_kwds', kwds)
 
-    def copy(self, deep=False):
-        """Return a deep copy of the object"""
-        args = self._args
-        kwds = self._kwds
+    def copy(self, deep=True, ignore=()):
+        """Return a copy of the object
 
+        Parameters
+        ----------
+        deep : boolean, optional
+            if True (default) then return a deep copy of all dict, list, and
+            SchemaBase objects within the object structure
+        ignore : list, optional
+            A list of keys for which the contents should not be copied, but
+            only stored by reference.
+        """
+        def _deep_copy(obj, ignore=()):
+            if isinstance(obj, SchemaBase):
+                args = tuple(_deep_copy(arg) for arg in obj._args)
+                kwds = {k: (_deep_copy(v, ignore=ignore)
+                            if k not in ignore else v)
+                        for k, v in obj._kwds.items()}
+                return obj.__class__(*args, **kwds)
+            elif isinstance(obj, list):
+                return [_deep_copy(v, ignore=ignore) for v in obj]
+            elif isinstance(obj, dict):
+                return {k: (_deep_copy(v, ignore=ignore)
+                            if k not in ignore else v)
+                        for k, v in obj.items()}
+            else:
+                return obj
         if deep:
-            import copy
-            args = copy.deepcopy(args)
-            kwds = copy.deepcopy(kwds)
-        return self.__class__(*args, **kwds)
+            return _deep_copy(self, ignore=ignore)
+        else:
+            return self.__class__(*self._args, **self._kwds)
 
     def __getattr__(self, item):
         # reminder: getattr is called after the normal lookups
@@ -119,7 +140,7 @@ class SchemaBase(object):
                               if k not in ignore})
         else:
             raise ValueError("{0} instance has both a value and properties : "
-                             "cannot serialize to dict")
+                             "cannot serialize to dict".format(self.__class__))
         if validate:
             self.validate(result)
         return result


Test patch:
diff --git a/tools/schemapi/tests/test_schemapi.py b/tools/schemapi/tests/test_schemapi.py
index f0b30113..eb5ce8ae 100644
--- a/tools/schemapi/tests/test_schemapi.py
+++ b/tools/schemapi/tests/test_schemapi.py
@@ -2,7 +2,7 @@ import jsonschema
 import pytest
 
 from .. import SchemaBase, schemaclass, Undefined
-from ..core import UndefinedType
+from ..schemapi import UndefinedType
 
 
 @schemaclass
@@ -204,3 +204,39 @@ def test_invalid_properties():
 
 def test_undefined_singleton():
     assert Undefined is UndefinedType()
+
+
+def test_copy():
+    dct = {'a': {'foo': 'bar'}, 'a2': {'foo': 42},
+       'b': ['a', 'b', 'c'], 'b2': [1, 2, 3], 'c': 42,
+       'd': ['x', 'y', 'z']}
+
+    myschema = MySchema.from_dict(dct)
+
+    # Make sure copy is deep
+    copy = myschema.copy(deep=True)
+    copy['a']['foo'] = 'new value'
+    copy['b'] = ['A', 'B', 'C']
+    copy['c'] = 164
+    assert myschema.to_dict() == dct
+
+    # If we ignore a value, changing the copy changes the original
+    copy = myschema.copy(deep=True, ignore=['a'])
+    copy['a']['foo'] = 'new value'
+    copy['b'] = ['A', 'B', 'C']
+    copy['c'] = 164
+    mydct = myschema.to_dict()
+    assert mydct['a']['foo'] == 'new value'
+    assert mydct['b'][0] == dct['b'][0]
+    assert mydct['c'] == dct['c']
+
+    # If copy is not deep, then changing copy below top level changes original
+    copy = myschema.copy(deep=False)
+    copy['a']['foo'] = 'baz'
+    copy['b'] = ['A', 'B', 'C']
+    copy['c'] = 164
+    mydct = myschema.to_dict()
+    assert mydct['a']['foo'] == 'baz'
+    assert mydct['b'] == dct['b']
+    assert mydct['c'] == dct['c']
+    
diff --git a/tools/schemapi/tests/test_utils.py b/tools/schemapi/tests/test_utils.py
index b12b7a06..cdeb944b 100644
--- a/tools/schemapi/tests/test_utils.py
+++ b/tools/schemapi/tests/test_utils.py
@@ -1,7 +1,7 @@
 import pytest
 
 from ..utils import resolve_references, get_valid_identifier
-from ..core import _FromDict
+from ..schemapi import _FromDict
 
 
 @pytest.fixture