-
-
Notifications
You must be signed in to change notification settings - Fork 402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DynamicMap with streams force callback on first lookup after refresh #858
Changes from 5 commits
74d7dca
7da2ea8
1b37619
ea79755
742116f
1edb409
f323413
0fea997
bcdebc1
7bae403
e97a530
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,3 +128,29 @@ def hierarchical(keys): | |
store1[v2].append(v1) | ||
hierarchies.append(store2 if hierarchy else {}) | ||
return hierarchies | ||
|
||
|
||
class enable_streams_cache(object): | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the discussion about, I would call it
I don't think it needs an
Would be reasonable. The issue with using either For this reason, maybe something like this would be clearer:
Just some things to think about... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The issue is that making context manager conditional is awkward, which is why I moved the boolean onto the object. I agree with renaming the argument and making it optional though. |
||
Context manager which temporarily enables lookup of frame in cache | ||
on a DynamicMap with streams. Allows passing any Dimensioned | ||
object which might contain a DynamicMap and whether to enable the | ||
cache. This allows looking up an item without triggering the | ||
callback. Useful when the object is looked up multiple times as | ||
part of some processing pipeline. | ||
""" | ||
|
||
def __init__(self, obj, enable): | ||
self.obj = obj | ||
self._enable = enable | ||
|
||
def __enter__(self): | ||
self.set_cache_flag(self._enable) | ||
|
||
def __exit__(self, exc_type, exc_val, exc_tb): | ||
self.set_cache_flag(False) | ||
|
||
def set_cache_flag(self, value): | ||
self.obj.traverse(lambda x: setattr(x, '_stream_cache_lookup', value), | ||
['DynamicMap']) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -787,7 +787,7 @@ def stream_parameters(streams, no_duplicates=True, exclude=['name']): | |
If no_duplicates is enabled, a KeyError will be raised if there are | ||
parameter name clashes across the streams. | ||
""" | ||
param_groups = [s.params().keys() for s in streams] | ||
param_groups = [s.contents.keys() for s in streams] | ||
names = [name for group in param_groups for name in group] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the fix! |
||
|
||
if no_duplicates: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
from ..core.options import Store, Compositor, SkipRendering | ||
from ..core.overlay import NdOverlay | ||
from ..core.spaces import HoloMap, DynamicMap | ||
from ..core.traversal import enable_streams_cache | ||
from ..element import Table | ||
from .util import (get_dynamic_mode, initialize_sampled, dim_axis_label, | ||
attach_streams) | ||
|
@@ -198,6 +199,7 @@ def __init__(self, keys=None, dimensions=None, layout_dimensions=None, | |
self.ranges = {} | ||
self.renderer = renderer if renderer else Store.renderers[self.backend].instance() | ||
self.comm = None | ||
self._force = True | ||
|
||
params = {k: v for k, v in params.items() | ||
if k in self.params()} | ||
|
@@ -258,6 +260,15 @@ def traverse(self, fn=None, specs=None, full_breadth=True): | |
if not full_breadth: break | ||
return accumulator | ||
|
||
@property | ||
def force(self): | ||
return self._force | ||
|
||
|
||
@force.setter | ||
def force(self, value): | ||
self.traverse(lambda x: setattr(x, '_force', value)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I see below, I think having There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We decided on replacing this with a |
||
|
||
def _frame_title(self, key, group_size=2, separator='\n'): | ||
""" | ||
|
@@ -481,6 +492,7 @@ def refresh(self, **kwargs): | |
Refreshes the plot by rerendering it and then pushing | ||
the updated data if the plot has an associated Comm. | ||
""" | ||
self.force = True | ||
if self.current_key: | ||
self.update(self.current_key) | ||
else: | ||
|
@@ -600,7 +612,9 @@ def _get_frame(self, key): | |
self.current_key = key | ||
return self.current_frame | ||
elif self.dynamic: | ||
key, frame = util.get_dynamic_item(self.hmap, self.dimensions, key) | ||
with enable_streams_cache(self.hmap, not self.force or not self.drawn): | ||
key, frame = util.get_dynamic_item(self.hmap, self.dimensions, key) | ||
self.force = False | ||
if not isinstance(key, tuple): key = (key,) | ||
key_map = dict(zip([d.name for d in self.hmap.kdims], key)) | ||
key = tuple(key_map.get(d.name, None) for d in self.dimensions) | ||
|
@@ -898,17 +912,16 @@ def get_extents(self, overlay, ranges): | |
class GenericCompositePlot(DimensionedPlot): | ||
|
||
def __init__(self, layout, keys=None, dimensions=None, **params): | ||
dynamic, sampled = get_dynamic_mode(layout) | ||
if sampled: | ||
initialize_sampled(layout, dimensions, keys[0]) | ||
|
||
if 'uniform' not in params: | ||
params['uniform'] = traversal.uniform(layout) | ||
|
||
top_level = keys is None | ||
if top_level: | ||
dimensions, keys = traversal.unique_dimkeys(layout) | ||
|
||
dynamic, sampled = get_dynamic_mode(layout) | ||
if sampled: | ||
initialize_sampled(layout, dimensions, keys[0]) | ||
self.layout = layout | ||
super(GenericCompositePlot, self).__init__(keys=keys, | ||
dynamic=dynamic, | ||
|
@@ -944,6 +957,11 @@ def _get_frame(self, key): | |
dim_keys = zip([d.name for d in self.dimensions | ||
if d in item.dimensions('key')], key) | ||
self.current_key = tuple(k[1] for k in dim_keys) | ||
elif item.traverse(lambda x: x, [DynamicMap]): | ||
with enable_streams_cache(item, not self.force or not self.drawn): | ||
key, frame = util.get_dynamic_item(item, self.dimensions, key) | ||
layout_frame[path] = frame | ||
continue | ||
elif self.uniform: | ||
dim_keys = zip([d.name for d in self.dimensions | ||
if d in item.dimensions('key')], key) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From our discussion, we think it ought to be something like:
That way the variable isn't about streams and you can disable cache lookup on dynamic maps not matter how they are (or aren't) using streams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we decided in the end that it is about streams.