Skip to content
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

Various fixes for bokeh static_source optimization #1413

Merged
merged 5 commits into from
May 28, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion holoviews/core/dimension.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from .options import Store, StoreOptions
from .pprint import PrettyPrinter

obj_id = id
# Alias parameter support for pickle loading

ALIASES = {'key_dimensions': 'kdims', 'value_dimensions': 'vdims',
Expand Down Expand Up @@ -479,7 +480,7 @@ class LabelledData(param.Parameterized):

_deep_indexable = False

def __init__(self, data, id=None, **params):
def __init__(self, data, id=None, plot_id=None, **params):
"""
All LabelledData subclasses must supply data to the
constructor, which will be held on the .data attribute.
Expand All @@ -488,6 +489,7 @@ def __init__(self, data, id=None, **params):
"""
self.data = data
self.id = id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever actually set the id via the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, probably for clone. Nevermind my question!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, clone does all the time.

self._plot_id = plot_id or obj_id(self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use __builtins__.id to avoid the name shadowing issue.

if isinstance(params.get('label',None), tuple):
(alias, long_name) = params['label']
label_sanitizer.add_aliases(**{alias:long_name})
Expand Down Expand Up @@ -531,6 +533,7 @@ def clone(self, data=None, shared_data=True, new_type=None, *args, **overrides):

if data is None and shared_data:
data = self.data
settings['plot_id'] = self._plot_id
# Apply name mangling for __ attribute
pos_args = getattr(self, '_' + type(self).__name__ + '__pos_params', [])
return clone_type(data, *args, **{k:v for k,v in settings.items()
Expand Down
2 changes: 1 addition & 1 deletion holoviews/core/layout.py
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ def __init__(self, items=None, identifier=None, parent=None, **kwargs):
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}
params = {p: kwargs.pop(p) for p in list(self.params().keys())+['id', 'plot_id'] if p in kwargs}
AttrTree.__init__(self, items, identifier, parent, **kwargs)
Dimensioned.__init__(self, self.data, **params)

Expand Down
1 change: 1 addition & 0 deletions holoviews/core/ndmapping.py
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,7 @@ def clone(self, data=None, shared_data=True, new_type=None, *args, **overrides):

if data is None and shared_data:
data = self.data
settings['plot_id'] = self._plot_id
# Apply name mangling for __ attribute
pos_args = getattr(self, '_' + type(self).__name__ + '__pos_params', [])
with item_check(not shared_data and self._check_items):
Expand Down
1 change: 1 addition & 0 deletions holoviews/core/spaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,7 @@ def clone(self, data=None, shared_data=True, new_type=None, link_inputs=True,
"""
if data is None and shared_data:
data = self.data
overrides['plot_id'] = self._plot_id
clone = super(UniformNdMapping, self).clone(overrides.pop('callback', self.callback),
shared_data, new_type,
*(data,) + args, **overrides)
Expand Down
2 changes: 2 additions & 0 deletions holoviews/element/path.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ def clone(self, *args, **overrides):
containing the specified args and kwargs.
"""
settings = dict(self.get_param_values(), **overrides)
if not args:
settings['plot_id'] = self._plot_id
return self.__class__(*args, **settings)


Expand Down
7 changes: 4 additions & 3 deletions holoviews/plotting/bokeh/element.py
Original file line number Diff line number Diff line change
Expand Up @@ -804,15 +804,16 @@ def update_frame(self, key, ranges=None, plot=None, element=None, empty=False):
# Cache frame object id to skip updating data if unchanged
previous_id = self.handles.get('previous_id', None)
if self.batched:
current_id = sum(element.traverse(lambda x: id(x.data), [Element]))
current_id = sum(element.traverse(lambda x: x._plot_id, [Element]))
else:
current_id = id(element.data)
current_id = element._plot_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it no longer taking into account the id of the data? It seems to do that in tabular...

self.handles['previous_id'] = current_id
self.static_source = self.dynamic and (current_id == previous_id)
self.static_source = (self.dynamic and (current_id == previous_id))
if self.batched:
data, mapping = self.get_batched_data(element, ranges, empty)
else:
data, mapping = self.get_data(element, ranges, empty)

if not self.static_source:
self._update_datasource(source, data)

Expand Down
2 changes: 1 addition & 1 deletion holoviews/plotting/bokeh/plot.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ def sync_sources(self):
from the same object.
"""
get_sources = lambda x: (id(x.current_frame.data), x)
filter_fn = lambda x: (x.shared_datasource and x.current_frame and
filter_fn = lambda x: (x.shared_datasource and x.current_frame is not None and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm: is this a a fix unrelated to the optimization itself?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems so, not sure why I pushed it here, but it is an important fix.

not isinstance(x.current_frame.data, np.ndarray)
and 'source' in x.handles)
data_sources = self.traverse(get_sources, [filter_fn])
Expand Down
7 changes: 4 additions & 3 deletions holoviews/plotting/bokeh/tabular.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,12 @@ def current_handles(self):
if self.static and not self.dynamic:
return handles


element = self.current_frame
previous_id = self.handles.get('previous_id', None)
current_id = id(self.current_frame.data) if self.current_frame else None
current_id = None if self.current_frame is None else id(element)+id(element.data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summing ids is not a safe test for uniqueness as many pairs of ids could sum to the same value (even if it is unlikely!). Note that collisions might not even be that rare as the id doesn't return a hash - it is more like to a memory address.

To check the identity of both the element and the data I would use the tuple (id(element), id(element.data)) and not the sum.

for handle in self._update_handles:
if (handle == 'source' and self.dynamic and
current_id == previous_id):
if (handle == 'source' and self.dynamic and current_id == previous_id):
continue
if handle in self.handles:
handles.append(self.handles[handle])
Expand Down