Repository: altair-viz/altair (python)

Issue:
Remove default width/height from Chart?
Having defaults causes issues with faceted plots (which become comically large) and with things like ``rangeStep``, which are ignored if ``width`` and ``height`` are set (see #481).

I would propose removing these defaults.

The disadvantage is that straightforward charts end up being smaller than is perhaps ideal for display in the notebook.

Pull request:
Change default width/height to config.view.width/height
Closes #482 

Patch:
diff --git a/altair/utils/core.py b/altair/utils/core.py
index 9ed12ea4..cb016dd7 100644
--- a/altair/utils/core.py
+++ b/altair/utils/core.py
@@ -3,6 +3,8 @@ Utility routines
 """
 import re
 import warnings
+import collections
+from copy import deepcopy
 
 import six
 import pandas as pd
@@ -296,3 +298,42 @@ def update_subtraits(obj, attrs, **kwargs):
             trait = dct[attr] = {}
         dct[attr] = update_subtraits(trait, attrs[1:], **kwargs)
     return obj
+
+
+def update_nested(original, update, copy=False):
+    """Update nested dictionaries
+
+    Parameters
+    ----------
+    original : dict
+        the original (nested) dictionary, which will be updated in-place
+    update : dict
+        the nested dictionary of updates
+    copy : bool, default False
+        if True, then copy the original dictionary rather than modifying it
+
+    Returns
+    -------
+    original : dict
+        a reference to the (modified) original dict
+
+    Examples
+    --------
+    >>> original = {'x': {'b': 2, 'c': 4}}
+    >>> update = {'x': {'b': 5, 'd': 6}, 'y': 40}
+    >>> update_nested(original, update)
+    >>> original
+    {'x': {'b': 5, 'c': 2, 'd': 6}, 'y': 40}
+    """
+    if copy:
+        original = deepcopy(original)
+    for key, val in update.items():
+        if isinstance(val, collections.Mapping):
+            orig_val = original.get(key, {})
+            if isinstance(orig_val, collections.Mapping):
+                original[key] = update_nested(orig_val, val)
+            else:
+                original[key] = val
+        else:
+            original[key] = val
+    return original
diff --git a/altair/vegalite/v2/api.py b/altair/vegalite/v2/api.py
index 403a4051..61bd00d7 100644
--- a/altair/vegalite/v2/api.py
+++ b/altair/vegalite/v2/api.py
@@ -6,7 +6,8 @@ from .schema import *
 from .schema import core, channels, Undefined
 
 from .data import data_transformers, pipe
-from ...utils import infer_vegalite_type, parse_shorthand_plus_data, use_signature
+from ...utils import (infer_vegalite_type, parse_shorthand_plus_data,
+                      use_signature, update_nested)
 from .display import renderers
 
 
@@ -133,6 +134,8 @@ def condition(predicate, if_true, if_false):
 # Top-level objects
 
 class TopLevelMixin(object):
+    _default_spec_values = {"config": {"view": {"width": 400, "height": 300}}}
+
     def _prepare_data(self):
         if isinstance(self.data, (dict, core.Data, core.InlineData,
                                   core.UrlData, core.NamedData)):
@@ -143,27 +146,31 @@ class TopLevelMixin(object):
             self.data = core.UrlData(self.data)
 
     def to_dict(self, *args, **kwargs):
-        # TODO: it's a bit weird that to_dict modifies the object.
-        #       Should we create a copy first?
-        original_data = getattr(self, 'data', Undefined)
-        self._prepare_data()
+        copy = self.copy()
+        original_data = getattr(copy, 'data', Undefined)
+        copy._prepare_data()
 
         # We make use of two context markers:
         # - 'data' points to the data that should be referenced for column type
         #   inference.
-        # - 'toplevel' is a boolean flag that is assumed to be true; if it's
+        # - 'top_level' is a boolean flag that is assumed to be true; if it's
         #   true then a "$schema" arg is added to the dict.
         context = kwargs.get('context', {}).copy()
+        is_top_level = context.get('top_level', True)
+
+        context['top_level'] = False
         if original_data is not Undefined:
             context['data'] = original_data
-        if context.get('top_level', True):
-            # since this is top-level we add $schema if it's missing
-            if '$schema' not in self._kwds:
-                self._kwds['$schema'] = SCHEMA_URL
-            # subschemas below this one are not top-level
-            context['top_level'] = False
         kwargs['context'] = context
-        return super(TopLevelMixin, self).to_dict(*args, **kwargs)
+
+        dct = super(TopLevelMixin, copy).to_dict(*args, **kwargs)
+
+        if is_top_level:
+            # since this is top-level we add $schema if it's missing
+            if '$schema' not in dct:
+                dct['$schema'] = SCHEMA_URL
+            dct = update_nested(copy._default_spec_values, dct, copy=True)
+        return dct
 
     # Layering and stacking
 
@@ -185,7 +192,7 @@ class TopLevelMixin(object):
 
 class Chart(TopLevelMixin, core.TopLevelFacetedUnitSpec):
     def __init__(self, data=Undefined, encoding=Undefined, mark=Undefined,
-                 width=400, height=300, **kwargs):
+                 width=Undefined, height=Undefined, **kwargs):
         super(Chart, self).__init__(data=data, encoding=encoding, mark=mark,
                                     width=width, height=height, **kwargs)
 


Test patch:
diff --git a/altair/utils/tests/test_core.py b/altair/utils/tests/test_core.py
index 904f39be..b2b597f1 100644
--- a/altair/utils/tests/test_core.py
+++ b/altair/utils/tests/test_core.py
@@ -5,7 +5,7 @@ import json
 import numpy as np
 import pandas as pd
 
-from .. import parse_shorthand, parse_shorthand_plus_data
+from .. import parse_shorthand, parse_shorthand_plus_data, update_nested
 
 
 def test_parse_shorthand():
@@ -57,3 +57,16 @@ def test_parse_shorthand_plus_data():
     check('z', data, field='z', type='temporal')
     check('count(x)', data, field='x', aggregate='count', type='quantitative')
     check('mean(*)', data, field='*', aggregate='mean')
+
+
+def test_update_nested():
+    original = {'x': {'b': {'foo': 2}, 'c': 4}}
+    update = {'x': {'b': {'foo': 5}, 'd': 6}, 'y': 40}
+
+    output = update_nested(original, update, copy=True)
+    assert output is not original
+    assert output == {'x': {'b': {'foo': 5}, 'c': 4, 'd': 6}, 'y': 40}
+
+    output2 = update_nested(original, update)
+    assert output2 is original
+    assert output == output2
diff --git a/altair/vegalite/tests/test_common.py b/altair/vegalite/tests/test_common.py
index 4034c62d..9ecc237b 100644
--- a/altair/vegalite/tests/test_common.py
+++ b/altair/vegalite/tests/test_common.py
@@ -4,23 +4,35 @@ import pytest
 
 from .. import v1, v2
 
-
-@pytest.fixture
-def basic_spec():
-    return {
-        'data': {'url': 'data.csv'},
-        'mark': 'line',
-        'encoding': {
-            'color': {'type': 'nominal', 'field': 'color'},
-            'x': {'type': 'quantitative', 'field': 'xval'},
-            'y': {'type': 'ordinal', 'field': 'yval'}
-        },
-        'height': 300,
-        'width': 400
+v1_defaults = {
+    'width': 400,
+    'height': 300
+}
+
+v2_defaults = {
+    'config': {
+        'view':{
+            'height':300,
+            'width':400
+        }
     }
+}
+
+basic_spec = {
+    'data': {'url': 'data.csv'},
+    'mark': 'line',
+    'encoding': {
+        'color': {'type': 'nominal', 'field': 'color'},
+        'x': {'type': 'quantitative', 'field': 'xval'},
+        'y': {'type': 'ordinal', 'field': 'yval'}
+    },
+}
+
+spec_v1 = dict(v1_defaults, **basic_spec)
+spec_v2 = dict(v2_defaults, **basic_spec)
 
 
-@pytest.mark.parametrize('alt', [v1, v2])
+@pytest.mark.parametrize('alt,basic_spec', [(v1, spec_v1), (v2, spec_v2)])
 def test_basic_chart_to_dict(alt, basic_spec):
     chart = alt.Chart('data.csv').mark_line().encode(
         alt.X('xval:Q'),
@@ -36,7 +48,7 @@ def test_basic_chart_to_dict(alt, basic_spec):
     assert dct == basic_spec
 
 
-@pytest.mark.parametrize('alt', [v1, v2])
+@pytest.mark.parametrize('alt,basic_spec', [(v1, spec_v1), (v2, spec_v2)])
 def test_basic_chart_from_dict(alt, basic_spec):
     chart = alt.Chart.from_dict(basic_spec)
     dct = chart.to_dict()