From 17cd8cfd644a6eb545fb7a26bca08cf08995bce4 Mon Sep 17 00:00:00 2001 From: jlstevens Date: Mon, 25 Jul 2016 15:56:59 +0100 Subject: [PATCH 1/9] Disabled merge_keywords on Options --- holoviews/core/options.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/holoviews/core/options.py b/holoviews/core/options.py index 356bb503d6..0a9e53bc09 100644 --- a/holoviews/core/options.py +++ b/holoviews/core/options.py @@ -262,7 +262,7 @@ class Options(param.Parameterized): Optional specification of the options key name. For instance, key could be 'plot' or 'style'.""") - merge_keywords = param.Boolean(default=True, doc=""" + merge_keywords = param.Boolean(default=False, doc=""" Whether to merge with the existing keywords if the corresponding node already exists""") @@ -275,7 +275,7 @@ class Options(param.Parameterized): skipping over invalid keywords or not. May only be specified at the class level.""") - def __init__(self, key=None, allowed_keywords=None, merge_keywords=True, **kwargs): + def __init__(self, key=None, allowed_keywords=None, merge_keywords=False, **kwargs): invalid_kws = [] for kwarg in sorted(kwargs.keys()): From 325639a8f76742b7ee55a265d33e6d9d68820f29 Mon Sep 17 00:00:00 2001 From: jlstevens Date: Mon, 25 Jul 2016 16:49:11 +0100 Subject: [PATCH 2/9] Set merge_keywords back to True and disabled during construction --- holoviews/core/options.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/holoviews/core/options.py b/holoviews/core/options.py index 0a9e53bc09..b57a92f232 100644 --- a/holoviews/core/options.py +++ b/holoviews/core/options.py @@ -262,7 +262,7 @@ class Options(param.Parameterized): Optional specification of the options key name. For instance, key could be 'plot' or 'style'.""") - merge_keywords = param.Boolean(default=False, doc=""" + merge_keywords = param.Boolean(default=True, doc=""" Whether to merge with the existing keywords if the corresponding node already exists""") @@ -275,7 +275,7 @@ class Options(param.Parameterized): skipping over invalid keywords or not. May only be specified at the class level.""") - def __init__(self, key=None, allowed_keywords=None, merge_keywords=False, **kwargs): + def __init__(self, key=None, allowed_keywords=None, merge_keywords=True, **kwargs): invalid_kws = [] for kwarg in sorted(kwargs.keys()): @@ -435,7 +435,7 @@ def _merge_options(self, identifier, group_name, options): try: return (group_options(**override_kwargs) - if options.merge_keywords else Options(group_name, **override_kwargs)) + if False else Options(group_name, **override_kwargs)) except OptionError as e: raise OptionError(e.invalid_keyword, e.allowed_keywords, From a78f9c1b6a03bbc77013e02fbf8a52abbcf13315 Mon Sep 17 00:00:00 2001 From: jlstevens Date: Mon, 25 Jul 2016 17:53:03 +0100 Subject: [PATCH 3/9] New child nodes no longer inherit kwargs from parent node --- holoviews/core/options.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/holoviews/core/options.py b/holoviews/core/options.py index b57a92f232..814ae64364 100644 --- a/holoviews/core/options.py +++ b/holoviews/core/options.py @@ -430,12 +430,16 @@ def _merge_options(self, identifier, group_name, options): if group_name not in self.groups: raise KeyError("Group %s not defined on SettingTree" % group_name) - current_node = self[identifier] if identifier in self.children else self - group_options = current_node.groups[group_name] - + if identifier in self.children: + current_node = self[identifier] + group_options = current_node.groups[group_name] + else: + #When creating a node (nothing to merge with) ensure it is empty + group_options = Options(group_name, + allowed_keywords=self.groups[group_name].allowed_keywords) try: return (group_options(**override_kwargs) - if False else Options(group_name, **override_kwargs)) + if options.merge_keywords else Options(group_name, **override_kwargs)) except OptionError as e: raise OptionError(e.invalid_keyword, e.allowed_keywords, @@ -465,7 +469,9 @@ def __getattr__(self, identifier): if valid_id in self.children: return self.__dict__[valid_id] - self.__setattr__(identifier, self.groups) + # When creating a intermediate child node, leave kwargs empty + self.__setattr__(identifier, {k:Options(k, allowed_keywords=v.allowed_keywords) + for k,v in self.groups.items()}) return self[identifier] From a22d18f5cb49527aa8e84bee6eabd089f08decb2 Mon Sep 17 00:00:00 2001 From: jlstevens Date: Mon, 25 Jul 2016 18:25:39 +0100 Subject: [PATCH 4/9] Closest method now uses most specific target in options call --- holoviews/core/options.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/holoviews/core/options.py b/holoviews/core/options.py index 814ae64364..2cdeb17311 100644 --- a/holoviews/core/options.py +++ b/holoviews/core/options.py @@ -542,7 +542,7 @@ def closest(self, obj, group): components = (obj.__class__.__name__, group_sanitizer(obj.group), label_sanitizer(obj.label)) - return self.find(components).options(group) + return self.find(components).options(group, target='.'.join(components)) From c585a000f02dc9720810ee7a767bbc37ae6e7f11 Mon Sep 17 00:00:00 2001 From: Philipp Rudiger Date: Mon, 25 Jul 2016 22:24:11 +0100 Subject: [PATCH 5/9] Build valid target spec in OptionsTree.closest --- holoviews/core/options.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/holoviews/core/options.py b/holoviews/core/options.py index 2cdeb17311..d7cc4216d9 100644 --- a/holoviews/core/options.py +++ b/holoviews/core/options.py @@ -542,7 +542,8 @@ def closest(self, obj, group): components = (obj.__class__.__name__, group_sanitizer(obj.group), label_sanitizer(obj.label)) - return self.find(components).options(group, target='.'.join(components)) + target = '.'.join([c for c in components if c]) + return self.find(components).options(group, target=target) From 2d44e99c1723b17499189c013d54a9a3932fab13 Mon Sep 17 00:00:00 2001 From: jlstevens Date: Tue, 26 Jul 2016 20:44:17 +0100 Subject: [PATCH 6/9] Added backend imports needed to run testoptions.py in isolation --- tests/testoptions.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/testoptions.py b/tests/testoptions.py index dc124cb24a..a158f4253a 100644 --- a/tests/testoptions.py +++ b/tests/testoptions.py @@ -9,6 +9,18 @@ Options.skip_invalid = False +try: + # Needed a backend to register backend and options + from holoviews.plotting import mpl +except: + pass + +try: + # Needed to register backend and options + from holoviews.plotting import bokeh +except: + pass + class TestOptions(ComparisonTestCase): def test_options_init(self): From 354f3dabcee3b85655ca438a9123341c1895b0ab Mon Sep 17 00:00:00 2001 From: jlstevens Date: Tue, 26 Jul 2016 20:45:15 +0100 Subject: [PATCH 7/9] Added seven more unit tests to check dynamic inheritance --- tests/testoptions.py | 161 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/tests/testoptions.py b/tests/testoptions.py index a158f4253a..60f89f606c 100644 --- a/tests/testoptions.py +++ b/tests/testoptions.py @@ -192,6 +192,167 @@ def test_optiontree_inheritance_flipped(self): {'kw2':'value2', 'kw4':'value4'}) +class TestStoreInheritanceDynamic(ComparisonTestCase): + """ + Tests to prevent regression after fix in PR #646 + """ + + def setUp(self): + self.store_copy = OptionTree(sorted(Store.options().items()), + groups=['style', 'plot', 'norm']) + self.backend = 'matplotlib' + Store.current_backend = self.backend + super(TestStoreInheritanceDynamic, self).setUp() + + def tearDown(self): + Store.options(val=self.store_copy) + super(TestStoreInheritanceDynamic, self).tearDown() + + def initialize_option_tree(self): + Store.options(val=OptionTree(groups=['plot', 'style'])) + options = Store.options() + options.Image = Options('style', cmap='hot', interpolation='nearest') + return options + + def test_merge_keywords(self): + options = self.initialize_option_tree() + options.Image = Options('style', clims=(0, 0.5)) + + expected = {'clims': (0, 0.5), 'cmap': 'hot', 'interpolation': 'nearest'} + direct_kws = options.Image.groups['style'].kwargs + inherited_kws = options.Image.options('style').kwargs + self.assertEqual(direct_kws, expected) + self.assertEqual(inherited_kws, expected) + + def test_merge_keywords_disabled(self): + options = self.initialize_option_tree() + options.Image = Options('style', clims=(0, 0.5), merge_keywords=False) + + expected = {'clims': (0, 0.5)} + direct_kws = options.Image.groups['style'].kwargs + inherited_kws = options.Image.options('style').kwargs + self.assertEqual(direct_kws, expected) + self.assertEqual(inherited_kws, expected) + + def test_specification_general_to_specific_group(self): + """ + Test order of specification starting with general and moving + to specific + """ + if 'matplotlib' not in Store.renderers: + raise SkipTest("General to specific option test requires matplotlib") + + options = self.initialize_option_tree() + + obj = Image(np.random.rand(10,10), group='SomeGroup') + + options.Image = Options('style', cmap='viridis') + options.Image.SomeGroup = Options('style', alpha=0.2) + + expected = {'alpha': 0.2, 'cmap': 'viridis', 'interpolation': 'nearest'} + lookup = Store.lookup_options('matplotlib', obj, 'style') + + self.assertEqual(lookup.kwargs, expected) + # Check the tree is structured as expected + node1 = options.Image.groups['style'] + node2 = options.Image.SomeGroup.groups['style'] + + self.assertEqual(node1.kwargs, {'cmap': 'viridis', 'interpolation': 'nearest'}) + self.assertEqual(node2.kwargs, {'alpha': 0.2}) + + + def test_specification_general_to_specific_group_and_label(self): + """ + Test order of specification starting with general and moving + to specific + """ + if 'matplotlib' not in Store.renderers: + raise SkipTest("General to specific option test requires matplotlib") + + options = self.initialize_option_tree() + + obj = Image(np.random.rand(10,10), group='SomeGroup', label='SomeLabel') + + options.Image = Options('style', cmap='viridis') + options.Image.SomeGroup.SomeLabel = Options('style', alpha=0.2) + + expected = {'alpha': 0.2, 'cmap': 'viridis', 'interpolation': 'nearest'} + lookup = Store.lookup_options('matplotlib', obj, 'style') + + self.assertEqual(lookup.kwargs, expected) + # Check the tree is structured as expected + node1 = options.Image.groups['style'] + node2 = options.Image.SomeGroup.SomeLabel.groups['style'] + + self.assertEqual(node1.kwargs, {'cmap': 'viridis', 'interpolation': 'nearest'}) + self.assertEqual(node2.kwargs, {'alpha': 0.2}) + + def test_specification_specific_to_general_group(self): + """ + Test order of specification starting with a specific option and + then specifying a general one + """ + if 'matplotlib' not in Store.renderers: + raise SkipTest("General to specific option test requires matplotlib") + + options = self.initialize_option_tree() + options.Image.SomeGroup = Options('style', alpha=0.2) + + obj = Image(np.random.rand(10,10), group='SomeGroup') + options.Image = Options('style', cmap='viridis') + + expected = {'alpha': 0.2, 'cmap': 'viridis', 'interpolation': 'nearest'} + lookup = Store.lookup_options('matplotlib', obj, 'style') + + self.assertEqual(lookup.kwargs, expected) + # Check the tree is structured as expected + node1 = options.Image.groups['style'] + node2 = options.Image.SomeGroup.groups['style'] + + self.assertEqual(node1.kwargs, {'cmap': 'viridis', 'interpolation': 'nearest'}) + self.assertEqual(node2.kwargs, {'alpha': 0.2}) + + + def test_specification_specific_to_general_group_and_label(self): + """ + Test order of specification starting with general and moving + to specific + """ + if 'matplotlib' not in Store.renderers: + raise SkipTest("General to specific option test requires matplotlib") + + options = self.initialize_option_tree() + options.Image.SomeGroup.SomeLabel = Options('style', alpha=0.2) + obj = Image(np.random.rand(10,10), group='SomeGroup', label='SomeLabel') + + options.Image = Options('style', cmap='viridis') + expected = {'alpha': 0.2, 'cmap': 'viridis', 'interpolation': 'nearest'} + lookup = Store.lookup_options('matplotlib', obj, 'style') + + self.assertEqual(lookup.kwargs, expected) + # Check the tree is structured as expected + node1 = options.Image.groups['style'] + node2 = options.Image.SomeGroup.SomeLabel.groups['style'] + + self.assertEqual(node1.kwargs, {'cmap': 'viridis', 'interpolation': 'nearest'}) + self.assertEqual(node2.kwargs, {'alpha': 0.2}) + + def test_custom_to_default_inheritance(self): + options = self.initialize_option_tree() + options.Image.A.B = Options('style', alpha=0.2) + + obj = Image(np.random.rand(10, 10), group='A', label='B') + expected_obj = {'alpha': 0.2, 'cmap': 'hot', 'interpolation': 'nearest'} + obj_lookup = Store.lookup_options('matplotlib', obj, 'style') + self.assertEqual(obj_lookup.kwargs, expected_obj) + + # Customize this particular object + custom_obj = obj(style=dict(clims=(0, 0.5))) + expected_custom_obj = dict(clims=(0,0.5), **expected_obj) + custom_obj_lookup = Store.lookup_options('matplotlib', custom_obj, 'style') + self.assertEqual(custom_obj_lookup.kwargs, expected_custom_obj) + + class TestStoreInheritance(ComparisonTestCase): """ Tests to prevent regression after fix in 71c1f3a that resolves From 19574845f6712faa5b3329e8a5846c5867b7b879 Mon Sep 17 00:00:00 2001 From: jlstevens Date: Tue, 26 Jul 2016 21:10:08 +0100 Subject: [PATCH 8/9] Added docstring to unit test --- tests/testoptions.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/testoptions.py b/tests/testoptions.py index 60f89f606c..415bde7ffa 100644 --- a/tests/testoptions.py +++ b/tests/testoptions.py @@ -337,7 +337,11 @@ def test_specification_specific_to_general_group_and_label(self): self.assertEqual(node1.kwargs, {'cmap': 'viridis', 'interpolation': 'nearest'}) self.assertEqual(node2.kwargs, {'alpha': 0.2}) - def test_custom_to_default_inheritance(self): + def test_custom_call_to_default_inheritance(self): + """ + Checks customs inheritance backs off to default tree correctly + using __call__. + """ options = self.initialize_option_tree() options.Image.A.B = Options('style', alpha=0.2) From 20b05388158f4d7d2d74fa8448519a67145598d0 Mon Sep 17 00:00:00 2001 From: jlstevens Date: Tue, 26 Jul 2016 21:10:26 +0100 Subject: [PATCH 9/9] Added unit test to check %%opts magic inheritance --- tests/testoptions.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/testoptions.py b/tests/testoptions.py index 415bde7ffa..cc0b2186eb 100644 --- a/tests/testoptions.py +++ b/tests/testoptions.py @@ -356,6 +356,34 @@ def test_custom_call_to_default_inheritance(self): custom_obj_lookup = Store.lookup_options('matplotlib', custom_obj, 'style') self.assertEqual(custom_obj_lookup.kwargs, expected_custom_obj) + def test_custom_magic_to_default_inheritance(self): + """ + Checks customs inheritance backs off to default tree correctly + simulating the %%opts cell magic. + """ + if 'matplotlib' not in Store.renderers: + raise SkipTest("Custom magic inheritance test requires matplotlib") + options = self.initialize_option_tree() + options.Image.A.B = Options('style', alpha=0.2) + + obj = Image(np.random.rand(10, 10), group='A', label='B') + + # Before customizing... + expected_obj = {'alpha': 0.2, 'cmap': 'hot', 'interpolation': 'nearest'} + obj_lookup = Store.lookup_options('matplotlib', obj, 'style') + self.assertEqual(obj_lookup.kwargs, expected_obj) + + custom_tree = {0: OptionTree(groups=['plot', 'style', 'norm'], + style={'Image' : dict(clims=(0, 0.5))})} + Store._custom_options['matplotlib'] = custom_tree + obj.id = 0 # Manually set the id to point to the tree above + + # Customize this particular object + expected_custom_obj = dict(clims=(0,0.5), **expected_obj) + custom_obj_lookup = Store.lookup_options('matplotlib', obj, 'style') + self.assertEqual(custom_obj_lookup.kwargs, expected_custom_obj) + + class TestStoreInheritance(ComparisonTestCase): """