Merge pull request #1088 from ioam/overlay_fix

Fixed bug unpacking Overlay and Layout objects in Layout.from_values

holoviews/core/layout.py

@@ -7,6 +7,7 @@
 
 from functools import reduce
 from itertools import chain
+from collections import defaultdict, Counter
 
 import numpy as np
 
@@ -15,8 +16,7 @@
 from .dimension import Dimension, Dimensioned, ViewableElement
 from .ndmapping import OrderedDict, NdMapping, UniformNdMapping
 from .tree import AttrTree
-from .util import (int_to_roman, sanitize_identifier, group_sanitizer,
-                   label_sanitizer, unique_array)
+from .util import (unique_array, get_path, make_path_unique)
 from . import traversal
 
 
@@ -27,7 +27,7 @@ class Composable(object):
     """
 
     def __add__(self, obj):
-        return Layout.from_values(self) + Layout.from_values(obj)
+        return Layout.from_values([self, obj])
 
 
     def __lshift__(self, other):
@@ -212,7 +212,7 @@ def __iter__(self):
 
 
     def __add__(self, obj):
-        return Layout.from_values(self) + Layout.from_values(obj)
+        return Layout.from_values([self, obj])
 
 
     def __len__(self):
@@ -268,7 +268,7 @@ def cols(self, n):
 
 
     def __add__(self, obj):
-        return Layout.from_values(self) + Layout.from_values(obj)
+        return Layout.from_values([self, obj])
 
 
     @property
@@ -314,6 +314,16 @@ class Layout(AttrTree, Dimensioned):
 
     _deep_indexable = True
 
+    def __init__(self, items=None, identifier=None, parent=None, **kwargs):
+        self.__dict__['_display'] = 'auto'
+        self.__dict__['_max_cols'] = 4
+        if items and all(isinstance(item, Dimensioned) for item in items):
+            items = self._process_items(items)
+        params = {p: kwargs.pop(p) for p in list(self.params().keys())+['id'] if p in kwargs}
+        AttrTree.__init__(self, items, identifier, parent, **kwargs)
+        Dimensioned.__init__(self, self.data, **params)
+
+
     @classmethod
     def collate(cls, data, kdims=None, key_dimensions=None):
         kdims = key_dimensions if (kdims is None) else kdims
@@ -329,76 +339,67 @@ def collate(cls, data, kdims=None, key_dimensions=None):
 
 
     @classmethod
-    def new_path(cls, path, item, paths, count):
-        sanitizers = [sanitize_identifier, group_sanitizer, label_sanitizer]
-        path = tuple(fn(p) for (p, fn) in zip(path, sanitizers))
-        while any(path[:i] in paths or path in [p[:i] for p in paths]
-                  for i in range(1,len(path)+1)):
-            path = path[:2]
-            pl = len(path)
-            if (pl == 1 and not item.label) or (pl == 2 and item.label):
-                new_path = path + (int_to_roman(count-1),)
-                if path in paths:
-                    paths[paths.index(path)] = new_path
-                path = path + (int_to_roman(count),)
-            else:
-                path = path[:-1] + (int_to_roman(count),)
-            count += 1
-        return path, count
+    def from_values(cls, vals):
+        """
+        Returns a Layout given a list (or tuple) of viewable
+        elements or just a single viewable element.
+        """
+        return cls(items=cls._process_items(vals))
 
 
     @classmethod
-    def relabel_item_paths(cls, items):
+    def _process_items(cls, vals):
         """
-        Given a list of path items (list of tuples where each element
-        is a (path, element) pair), generate a new set of path items that
-        guarantees that no paths clash. This uses the element labels as
-        appropriate and automatically generates roman numeral
-        identifiers if necessary.
+        Processes a list of Labelled types unpacking any objects of
+        the same type (e.g. a Layout) and finding unique paths for
+        all the items in the list.
         """
-        paths, path_items = [], []
-        count = 2
-        for path, item in items:
-            new_path, count = cls.new_path(path, item, paths, count)
-            new_path = tuple(''.join((p[0].upper(), p[1:])) for p in new_path)
-            path_items.append(item)
-            paths.append(new_path)
-        return list(zip(paths, path_items))
+        if type(vals) is cls:
+            return vals.data
+        elif not isinstance(vals, (list, tuple)):
+            vals = [vals]
+        paths = cls._initial_paths(vals)
+        path_counter = Counter(paths)
+        items = []
+        counts = defaultdict(lambda: 1)
+        counts.update({k: 1 for k, v in path_counter.items() if v > 1})
+        cls._unpack_paths(vals, items, counts)
+        return items
 
 
     @classmethod
-    def from_values(cls, val):
+    def _initial_paths(cls, items, paths=None):
         """
-        Returns a Layout given a list (or tuple) of viewable
-        elements or just a single viewable element.
+        Recurses the passed items finding paths for each. Useful for
+        determining which paths are not unique and have to be resolved.
         """
-        collection = isinstance(val, (list, tuple))
-        if type(val) is cls:
-            return val
-        elif not collection:
-            val = [val]
-        paths, items = [], []
-        count = 2
-        for v in val:
-            group = group_sanitizer(v.group)
-            group = ''.join([group[0].upper(), group[1:]])
-            label = label_sanitizer(v.label if v.label else 'I')
-            label = ''.join([label[0].upper(), label[1:]])
-            new_path, count = cls.new_path((group, label), v, paths, count)
-            new_path = tuple(''.join((p[0].upper(), p[1:])) for p in new_path)
-            paths.append(new_path)
-            items.append((new_path, v))
-        return cls(items=items)
+        if paths is None:
+            paths = []
+        for item in items:
+            path, item = item if isinstance(item, tuple) else (None, item)
+            if type(item) is cls:
+                cls._initial_paths(item.items(), paths)
+                continue
+            paths.append(get_path(item))
+        return paths
 
 
-    def __init__(self, items=None, identifier=None, parent=None, **kwargs):
-        self.__dict__['_display'] = 'auto'
-        self.__dict__['_max_cols'] = 4
-        if items and all(isinstance(item, Dimensioned) for item in items):
-            items = self.from_values(items).data
-        params = {p: kwargs.pop(p) for p in list(self.params().keys())+['id'] if p in kwargs}
-        AttrTree.__init__(self, items, identifier, parent, **kwargs)
-        Dimensioned.__init__(self, self.data, **params)
+    @classmethod
+    def _unpack_paths(cls, objs, items, counts):
+        """
+        Recursively unpacks lists and Layout-like objects, accumulating
+        into the supplied list of items.
+        """
+        if type(objs) is cls:
+            objs = objs.items()
+        for item in objs:
+            path, obj = item if isinstance(item, tuple) else (None, item)
+            if type(obj) is cls:
+                cls._unpack_paths(obj, items, counts)
+                continue
+            path = get_path(item)
+            new_path = make_path_unique(path, counts)
+            items.append((new_path, obj))
 
 
     @property
@@ -502,9 +503,7 @@ def __len__(self):
 
 
     def __add__(self, other):
-        other = self.from_values(other)
-        items = list(self.data.items()) + list(other.data.items())
-        return Layout(items=self.relabel_item_paths(items)).display('all')
+        return Layout.from_values([self, other]).display('all')
 
 
 

holoviews/core/overlay.py

@@ -36,10 +36,7 @@ def dynamic_mul(*args, **kwargs):
             items = [(k, self * v) for (k, v) in other.items()]
             return other.clone(items)
 
-        self_item = [((self.group, self.label if self.label else 'I'), self)]
-        other_items = (other.items() if isinstance(other, Overlay)
-                       else [((other.group, other.label if other.label else 'I'), other)])
-        return Overlay(items=Overlay.relabel_item_paths(list(self_item) + list(other_items)))
+        return Overlay.from_values([self, other])
 
 
 
@@ -103,11 +100,6 @@ class Overlay(Layout, CompositeOverlay):
     Layout and CompositeOverlay.
     """
 
-    @classmethod
-    def _from_values(cls, val):
-        return reduce(lambda x,y: x*y, val).map(lambda x: x.display('auto'), [Overlay])
-
-
     def __init__(self, items=None, group=None, label=None, **params):
         view_params = ViewableElement.params().keys()
         self.__dict__['_fixed'] = False
@@ -138,19 +130,13 @@ def get(self, identifier, default=None):
 
 
     def __add__(self, other):
-        return Layout.from_values(self) + Layout.from_values(other)
+        return Layout.from_values([self, other])
 
 
     def __mul__(self, other):
-        if isinstance(other, Overlay):
-            items = list(self.data.items()) + list(other.data.items())
-        elif isinstance(other, ViewableElement):
-            label = other.label if other.label else 'I'
-            items = list(self.data.items()) + [((other.group, label), other)]
-        elif isinstance(other, UniformNdMapping):
+        if not isinstance(other, ViewableElement):
             raise NotImplementedError
-
-        return Overlay(items=self.relabel_item_paths(items)).display('all')
+        return Overlay.from_values([self, other])
 
 
     def collate(self):

holoviews/core/spaces.py

@@ -223,7 +223,7 @@ def dynamic_mul(*args, **kwargs):
 
 
     def __add__(self, obj):
-        return Layout.from_values(self) + Layout.from_values(obj)
+        return Layout.from_values([self, obj])
 
 
     def __lshift__(self, other):
@@ -1137,7 +1137,7 @@ def __len__(self):
 
 
     def __add__(self, obj):
-        return Layout.from_values(self) + Layout.from_values(obj)
+        return Layout.from_values([self, obj])
 
 
     @property

holoviews/core/util.py

@@ -4,7 +4,7 @@
 import string, fnmatch
 import unicodedata
 import datetime as dt
-from collections import defaultdict
+from collections import defaultdict, Counter
 
 import numpy as np
 import param
@@ -1002,6 +1002,46 @@ def unpack_group(group, getter):
             yield (wrap_tuple(key), obj)
 
 
+def capitalize(string):
+    """
+    Capitalizes the first letter of a string.
+    """
+    return string[0].upper() + string[1:]
+
+
+def get_path(item):
+    """
+    Gets a path from an Labelled object or from a tuple of an existing
+    path and a labelled object. The path strings are sanitized and
+    capitalized.
+    """
+    sanitizers = [group_sanitizer, label_sanitizer]
+    if isinstance(item, tuple):
+        path, item = item
+        if item.label:
+            if len(path) > 1 and item.label == path[1]:
+                path = path[:2]
+            else:
+                path = path[:1] + (item.label,)
+        else:
+            path = path[:1]
+    else:
+        path = (item.group, item.label) if item.label else (item.group,)
+    return tuple(capitalize(fn(p)) for (p, fn) in zip(path, sanitizers))
+
+
+def make_path_unique(path, counts):
+    """
+    Given a path, a list of existing paths and counts for each of the
+    existing paths.
+    """
+    while path in counts:
+        count = counts[path]
+        counts[path] += 1
+        path = path + (int_to_roman(count),)
+    if len(path) == 1:
+        path = path + (int_to_roman(counts.get(path, 1)),)
+    return path
 
 
 class ndmapping_groupby(param.ParameterizedFunction):

tests/testcomposites.py

@@ -126,6 +126,31 @@ def test_layouttree_quadruple_2(self):
                                     ('Element', 'LabelA', 'III'),
                                     ('Element', 'LabelA', 'IV')])
 
+    def test_layout_from_values_with_layouts(self):
+        layout1 = self.el1 + self.el4
+        layout2 = self.el2 + self.el5
+        paths = Layout.from_values([layout1, layout2]).keys()
+        self.assertEqual(paths, [('Element', 'I'), ('ValA', 'I'),
+                                 ('Element', 'II'), ('ValB', 'I')])
+
+    def test_layout_from_values_with_mixed_types(self):
+        layout1 = self.el1 + self.el4 + self.el7
+        layout2 = self.el2 + self.el5 + self.el8
+        paths = Layout.from_values([layout1, layout2, self.el3]).keys()
+        self.assertEqual(paths, [('Element', 'I'), ('ValA', 'I'),
+                                 ('ValA', 'LabelA'), ('Element', 'II'),
+                                 ('ValB', 'I'), ('ValA', 'LabelB'),
+                                 ('Element', 'III')])
+
+    def test_layout_from_values_retains_custom_path(self):
+        layout = Layout([('Custom', self.el1)])
+        paths = Layout.from_values([layout, self.el2]).keys()
+        self.assertEqual(paths, [('Custom', 'I'), ('Element', 'I')])
+
+    def test_layout_from_values_retains_custom_path_with_label(self):
+        layout = Layout([('Custom', self.el6)])
+        paths = Layout.from_values([layout, self.el2]).keys()
+        self.assertEqual(paths, [('Custom', 'LabelA'), ('Element', 'I')])
 
 
 class OverlayTestCase(ElementTestCase):
@@ -253,6 +278,31 @@ def test_overlay_quadruple_2(self):
                                     ('Element', 'LabelA', 'III'),
                                     ('Element', 'LabelA', 'IV')])
 
+    def test_overlay_from_values_with_layouts(self):
+        layout1 = self.el1 + self.el4
+        layout2 = self.el2 + self.el5
+        paths = Layout.from_values([layout1, layout2]).keys()
+        self.assertEqual(paths, [('Element', 'I'), ('ValA', 'I'),
+                                 ('Element', 'II'), ('ValB', 'I')])
+
+    def test_overlay_from_values_with_mixed_types(self):
+        overlay1 = self.el1 + self.el4 + self.el7
+        overlay2 = self.el2 + self.el5 + self.el8
+        paths = Layout.from_values([overlay1, overlay2, self.el3]).keys()
+        self.assertEqual(paths, [('Element', 'I'), ('ValA', 'I'),
+                                 ('ValA', 'LabelA'), ('Element', 'II'),
+                                 ('ValB', 'I'), ('ValA', 'LabelB'),
+                                 ('Element', 'III')])
+
+    def test_overlay_from_values_retains_custom_path(self):
+        overlay = Overlay([('Custom', self.el1)])
+        paths = Overlay.from_values([overlay, self.el2]).keys()
+        self.assertEqual(paths, [('Custom', 'I'), ('Element', 'I')])
+
+    def test_overlay_from_values_retains_custom_path_with_label(self):
+        overlay = Overlay([('Custom', self.el6)])
+        paths = Overlay.from_values([overlay, self.el2]).keys()
+        self.assertEqual(paths, [('Custom', 'LabelA'), ('Element', 'I')])