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

Fixed dynamic stream sources assignment in plotting code #1297

Merged
merged 18 commits into from Apr 17, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions holoviews/core/operation.py
Expand Up @@ -103,6 +103,14 @@ class ElementOperation(Operation):
first component is a Normalization.ranges list and the second
component is Normalization.keys. """)

link_inputs = param.Boolean(default=False, doc="""
If the operation is dynamic, whether or not linked streams
should be transferred from the operation inputs for backends
that support linked streams. For example if an operation is
applied to a DynamicMap with an RangeXY determines if the
output of the operation will update the stream on the input
with current axis ranges.""")
Copy link
Contributor

@jlstevens jlstevens Apr 17, 2017

Choose a reason for hiding this comment

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

Much better!

Minor edits:

For example if an operation is applied to a DynamicMap with an RangeXY, this switch determines
whether the corresponding visualization should update this range stream with range changes originating
from the newly generated axes.


Copy link
Contributor

@jlstevens jlstevens Apr 17, 2017

Choose a reason for hiding this comment

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

Needs a lot more explanation as almost none of the required concepts exist at this level:

  1. An operation doesn't create plots, it generates a data-structure.
  2. Specifically, linking is about streams which relates to DynamicMap so this should say something about when the operation is dynamic.
  3. Linked streams only exist for specific backends.

As a very rough suggestion, it should be something along the lines of:

'If the operation is dynamic, this switch specifies whether or not linked streams should be defined from the operation inputs. Only applies when rendering the output with backends that support linked streams. For example ... '

streams = param.List(default=[], doc="""
List of streams that are applied if dynamic=True, allowing
for dynamic interaction with the plot.""")
Expand Down Expand Up @@ -139,8 +147,8 @@ def __call__(self, element, **params):
processed = element.clone(grid_data)
elif dynamic:
from ..util import Dynamic
streams = getattr(self.p, 'streams', [])
processed = Dynamic(element, streams=streams,
processed = Dynamic(element, streams=self.p.streams,
link_inputs=self.p.link.inputs,
operation=self, kwargs=params)
elif isinstance(element, ViewableElement):
processed = self._process(element)
Expand Down
13 changes: 11 additions & 2 deletions holoviews/core/overlay.py
Expand Up @@ -29,6 +29,7 @@ def dynamic_mul(*args, **kwargs):
element = other[args]
return self * element
callback = Callable(dynamic_mul, inputs=[self, other])
callback._is_overlay = True
return other.clone(shared_data=False, callback=callback,
streams=[])
if isinstance(other, UniformNdMapping) and not isinstance(other, CompositeOverlay):
Expand All @@ -41,7 +42,6 @@ def dynamic_mul(*args, **kwargs):




class CompositeOverlay(ViewableElement, Composable):
"""
CompositeOverlay provides a common baseclass for Overlay classes.
Expand Down Expand Up @@ -136,7 +136,16 @@ def __add__(self, other):


def __mul__(self, other):
if not isinstance(other, ViewableElement):
if type(other).__name__ == 'DynamicMap':
from .spaces import Callable
def dynamic_mul(*args, **kwargs):
element = other[args]
return self * element
callback = Callable(dynamic_mul, inputs=[self, other])
callback._is_overlay = True
return other.clone(shared_data=False, callback=callback,
streams=[])
elif not isinstance(other, ViewableElement):
Copy link
Contributor

Choose a reason for hiding this comment

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

We really need to refactor __mul__ (dynamic and non-dynamic) into an operation for 2.0. It was bad enough before considering dynamic and now we have _dynamic_mul methods and dynamic_mul inner functions all over the place.

raise NotImplementedError
return Overlay.from_values([self, other])

Expand Down
44 changes: 24 additions & 20 deletions holoviews/core/spaces.py
Expand Up @@ -117,29 +117,19 @@ def _dynamic_mul(self, dimensions, other, keys):
map_obj = self if isinstance(self, DynamicMap) else other

def dynamic_mul(*key, **kwargs):
key_map = {d.name: k for d, k in zip(dimensions, key)}
layers = []
try:
if isinstance(self, DynamicMap):
safe_key = () if not self.kdims else key
_, self_el = util.get_dynamic_item(self, dimensions, safe_key)
if self_el is not None:
layers.append(self_el)
else:
layers.append(self[key])
self_el = self.select(**key_map) if self.kdims else self[()]
except KeyError:
pass
try:
if isinstance(other, DynamicMap):
safe_key = () if not other.kdims else key
_, other_el = util.get_dynamic_item(other, dimensions, safe_key)
if other_el is not None:
layers.append(other_el)
else:
layers.append(other[key])
other_el = other.select(**key_map) if other.kdims else other[()]
except KeyError:
pass
return Overlay(layers)
callback = Callable(dynamic_mul, inputs=[self, other])
callback._is_overlay = True
if map_obj:
return map_obj.clone(callback=callback, shared_data=False,
kdims=dimensions, streams=[])
Expand Down Expand Up @@ -207,6 +197,7 @@ def dynamic_mul(*args, **kwargs):
element = self[args]
return element * other
callback = Callable(dynamic_mul, inputs=[self, other])
callback._is_overlay = True
return self.clone(shared_data=False, callback=callback,
streams=[])
items = [(k, v * other) for (k, v) in self.data.items()]
Expand Down Expand Up @@ -413,7 +404,11 @@ class Callable(param.Parameterized):
when composite objects such as Layouts are returned from the
callback. This is required for building interactive, linked
visualizations (for the backends that support them) when returning
Layouts, NdLayouts or GridSpace objects.
Layouts, NdLayouts or GridSpace objects. When chaining multiple
DynamicMaps into a pipeline, the link_inputs parameter declares
whether the visualization generated from this Callable will
inherit the linked streams. This parameter is used as a hint by
the applicable backend.
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this read a bit better:

the link_inputs parameter declares whether the visualization generated using this Callable


The mapping should map from an appropriate key to a list of
streams associated with the selected object. The appropriate key
Expand All @@ -429,6 +424,16 @@ class Callable(param.Parameterized):
The list of inputs the callable function is wrapping. Used
to allow deep access to streams in chained Callables.""")

link_inputs = param.Boolean(default=True, doc="""
If the Callable wraps around other DynamicMaps in its inputs,
determines if linked streams attached to the inputs areb
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: 'areb'

transferred to the objects returned by the Callable.

For example if the Callable wraps a DynamicMap with an
RangeXY stream determines if the output of the Callable will
update the stream on the input with current axis ranges for
backends that support linked streams.""")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use my earlier suggestion here as well without changing it much.


memoize = param.Boolean(default=True, doc="""
Whether the return value of the callable should be memoized
based on the call arguments and any streams attached to the
Expand All @@ -441,6 +446,8 @@ class Callable(param.Parameterized):
def __init__(self, callable, **params):
super(Callable, self).__init__(callable=callable, **params)
self._memoized = {}
self._is_overlay = False


@property
def argspec(self):
Expand Down Expand Up @@ -687,10 +694,7 @@ def clone(self, data=None, shared_data=True, new_type=None, *args, **overrides):
# Ensure the clone references this object to ensure
# stream sources are inherited
if clone.callback is self.callback:
clone.callback = self.callback.clone()
if self not in clone.callback.inputs:
with util.disable_constant(clone.callback):
clone.callback.inputs = clone.callback.inputs+[self]
clone.callback = clone.callback.clone(inputs=[self])
return clone


Expand Down Expand Up @@ -1104,7 +1108,7 @@ def dynamic_hist(obj):
adjoin=False, **kwargs)

from ..util import Dynamic
hist = Dynamic(self, operation=dynamic_hist)
hist = Dynamic(self, link_inputs=False, operation=dynamic_hist)
if adjoin:
return self << hist
else:
Expand Down
10 changes: 10 additions & 0 deletions holoviews/operation/datashader.py
Expand Up @@ -264,6 +264,11 @@ class shade(ElementOperation):
and any valid transfer function that accepts data, mask, nbins
arguments.""")

link_inputs = param.Boolean(default=True, doc="""
By default, the link_inputs parameter is set to True so that
when applying shade, backends that support linked streams
update RangeXY streams on the inputs of the shade operation.""")

@classmethod
def concatenate(cls, overlay):
"""
Expand Down Expand Up @@ -395,6 +400,11 @@ class dynspread(ElementOperation):
Higher values give more spreading, up to the max_px
allowed.""")

link_inputs = param.Boolean(default=True, doc="""
By default, the link_inputs parameter is set to True so that
when applying dynspread, backends that support linked streams
update RangeXY streams on the inputs of the dynspread operation.""")

Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue as for the shade operation.

@classmethod
def uint8_to_uint32(cls, img):
shape = img.shape
Expand Down
2 changes: 1 addition & 1 deletion holoviews/plotting/bokeh/callbacks.py
Expand Up @@ -500,7 +500,7 @@ def initialize(self):
if cb_hash in self._callbacks:
# Merge callbacks if another callback has already been attached
cb = self._callbacks[cb_hash]
cb.streams += self.streams
cb.streams = list(set(cb.streams+self.streams))
for k, v in self.handle_ids.items():
cb.handle_ids[k].update(v)
continue
Expand Down
16 changes: 13 additions & 3 deletions holoviews/plotting/bokeh/element.py
Expand Up @@ -27,7 +27,7 @@
from ...element import RGB
from ...streams import Stream, RangeXY, RangeX, RangeY
from ..plot import GenericElementPlot, GenericOverlayPlot
from ..util import dynamic_update, get_sources, attach_streams
from ..util import dynamic_update, attach_streams
from .plot import BokehPlot, TOOLS
from .util import (mpl_to_bokeh, convert_datetime, update_plot, get_tab_title,
bokeh_version, mplcmap_to_palette, py2js_tickformatter,
Expand Down Expand Up @@ -190,9 +190,18 @@ def _construct_callbacks(self):
Initializes any callbacks for streams which have defined
the plotted object as a source.
"""
if isinstance(self, OverlayPlot):
zorders = []
elif self.batched:
zorders = list(range(self.zorder, self.zorder+len(self.hmap.last)))
else:
zorders = [self.zorder]

if isinstance(self, OverlayPlot) and not self.batched:
sources = []
if not self.static or isinstance(self.hmap, DynamicMap):
sources = [(i, o) for i, o in get_sources(self.hmap)
if i in [None, self.zorder]]
sources = [(i, o) for i, inputs in self.stream_sources.items()
for o in inputs if i in zorders]
Copy link
Contributor

Choose a reason for hiding this comment

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

What does being a source have to do with a z-ordering? The latter is a visualization concept so what does that have to with being a source or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

See below

else:
sources = [(self.zorder, self.hmap.last)]
cb_classes = set()
Expand All @@ -208,6 +217,7 @@ def _construct_callbacks(self):
cbs.append(cb(self, cb_streams, source))
return cbs


def _hover_opts(self, element):
if self.batched:
dims = list(self.hmap.last.kdims)
Expand Down
13 changes: 10 additions & 3 deletions holoviews/plotting/plot.py
Expand Up @@ -21,7 +21,8 @@
from ..core.util import stream_parameters
from ..element import Table
from .util import (get_dynamic_mode, initialize_sampled, dim_axis_label,
attach_streams, traverse_setter, get_nested_streams)
attach_streams, traverse_setter, get_nested_streams,
compute_overlayable_zorders)


class Plot(param.Parameterized):
Expand Down Expand Up @@ -554,7 +555,7 @@ class GenericElementPlot(DimensionedPlot):

def __init__(self, element, keys=None, ranges=None, dimensions=None,
batched=False, overlaid=0, cyclic_index=0, zorder=0, style=None,
overlay_dims={}, **params):
overlay_dims={}, stream_sources=[], **params):
self.zorder = zorder
self.cyclic_index = cyclic_index
self.overlaid = overlaid
Expand All @@ -567,6 +568,11 @@ def __init__(self, element, keys=None, ranges=None, dimensions=None,
else:
self.hmap = element

if overlaid:
self.stream_sources = stream_sources
else:
self.stream_sources = compute_overlayable_zorders(self.hmap)

plot_element = self.hmap.last
if self.batched and not isinstance(self, GenericOverlayPlot):
plot_element = [el for el in plot_element if el][-1]
Expand Down Expand Up @@ -836,6 +842,7 @@ def _create_subplots(self, ranges):
ordering = util.layer_sort(self.hmap)
registry = Store.registry[self.renderer.backend]
batched = self.batched and type(self.hmap.last) is NdOverlay
stream_sources = self.stream_sources
if batched:
batchedplot = registry.get(type(self.hmap.last.last))
if (batched and batchedplot and 'batched' in batchedplot._plot_methods and
Expand Down Expand Up @@ -902,7 +909,7 @@ def _create_subplots(self, ranges):
layout_dimensions=self.layout_dimensions,
ranges=ranges, show_title=self.show_title,
style=style, uniform=self.uniform,
renderer=self.renderer,
renderer=self.renderer, stream_sources=stream_sources,
zorder=zorder, **passed_handles)

if not isinstance(key, tuple): key = (key,)
Expand Down
89 changes: 63 additions & 26 deletions holoviews/plotting/util.py
@@ -1,4 +1,5 @@
from __future__ import unicode_literals
from collections import defaultdict

import numpy as np
import param
Expand All @@ -7,7 +8,7 @@
Overlay, GridSpace, NdLayout, Store, Dataset)
from ..core.spaces import get_nested_streams, Callable
from ..core.util import (match_spec, is_number, wrap_tuple, basestring,
get_overlay_spec, unique_iterator)
get_overlay_spec, unique_iterator, unique_iterator)


def displayable(obj):
Expand Down Expand Up @@ -72,6 +73,67 @@ def collate(obj):
raise Exception(undisplayable_info(obj))


def isoverlay_fn(obj):
"""
Determines whether object is a DynamicMap returning (Nd)Overlay types.
"""
return isinstance(obj, DynamicMap) and (isinstance(obj.last, CompositeOverlay))


def compute_overlayable_zorders(obj, path=[]):
"""
Traverses the Callables on DynamicMap to determine which objects
Copy link
Contributor

@jlstevens jlstevens Apr 17, 2017

Choose a reason for hiding this comment

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

Isn't the traversal of DynamicMap one of the things it needs to do as opposed to the only thing it needs to do?

How about:

Traverses an overlayable composite container to determine which objects are associated with specific (Nd)Overlay layers by z-order, making sure to take DynamicMap Callables into account. Returns a mapping between the zorders of each layer and a corresponding lists of objects.

in the graph are associated with specific (Nd)Overlay layers.
Returns a mapping between the zorders of each layer and lists of
objects associated with each.

Used to determine which subplots should be linked with Stream
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe 'which overlaid subplots'?

callbacks.
"""
path = path+[obj]
zorder_map = defaultdict(list)

# Process non-dynamic layers
if not isinstance(obj, DynamicMap):
if isinstance(obj, CompositeOverlay):
for i, o in enumerate(obj):
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be more readable renaming the i local variable to z.

zorder_map[i] = [o, obj]
else:
if obj not in zorder_map[0]:
zorder_map[0].append(obj)
return zorder_map

isoverlay = isinstance(obj.last, CompositeOverlay)
Copy link
Contributor

Choose a reason for hiding this comment

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

As the previous conditional returned for non-DynamicMaps this must be a DynamicMap now and there is no guarantee that there is anything in the cache for .last. As long as you have anticipated that .last may be None - in which case isoverlay is False - then this should be ok...

Copy link
Member Author

Choose a reason for hiding this comment

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

The relevant nodes are evaluated by this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, just to be sure I would assert that .last is not None to make sure initial_key was called.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not all paths have to have been evaluated, that would break everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I agree.

isdynoverlay = obj.callback._is_overlay
found = any(isinstance(p, DynamicMap) and p.callback._overlay for p in path)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the definition of found can be moved just above where it is used.

if obj not in zorder_map[0] and not isoverlay:
zorder_map[0].append(obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would execute if .last is None. Maybe this is ok because you are assuming initial_key is called already?

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, this is in plotting, the graph has to have been evaluated by this point, even if certain (unimportant) intermediate nodes haven't been.


# Process the inputs of the DynamicMap callback
dmap_inputs = obj.callback.inputs if obj.callback.link_inputs else []
for i, inp in enumerate(dmap_inputs):
if any(not (isoverlay_fn(p) or p.last is None) for p in path) and isoverlay_fn(inp):
# Skips branches of graph that collapse Overlay layers
# to avoid adding layers that have been reduced or removed
continue
Copy link
Contributor

@jlstevens jlstevens Apr 17, 2017

Choose a reason for hiding this comment

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

What happens if you have an operation that takes in an overlay of 3 things and returns an overlay of 2 things?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now handled.


# Recurse into DynamicMap.callback.inputs and update zorder_map
Copy link
Contributor

Choose a reason for hiding this comment

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

So this would not happen for a DynamicMap callback with inputs that is user defined i.e isdynoverlay is False because _is_overlay wouldn't be set to True by the user? Not saying this is wrong, just making sure this is what we want...

Copy link
Member Author

Choose a reason for hiding this comment

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

So this would not happen for a DynamicMap callback with inputs that is user defined i.e isdynoverlay is False because _is_overlay wouldn't be set to True by the user?

That's correct, the branch below handles that by iterating over the items in the user defined DynamicMap, which will ensure the zorder is maintained but won't attempt to figure out linked inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. As we discussed, this should be ok as the user can set things up explicitly if they want to transfer sources around (which means they shouldn't ever need to use _is_overlay.

Seems fine.

i = i if isdynoverlay else 0
dinputs = compute_overlayable_zorders(inp, path=path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call this zinputs?

offset = max(zorder_map.keys())
for k, v in dinputs.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of k maybe this should be called zinp or something?

zorder_map[offset+k+i] = list(unique_iterator(zorder_map[offset+k+i]+v))

# If object branches but does not declare inputs (e.g. user defined
# DynamicMaps returning (Nd)Overlay) add the items on the DynamicMap.last
if found and isoverlay and not isdynoverlay:
offset = max(zorder_map.keys())
for i, o in enumerate(obj.last):
if o not in zorder_map[offset+i]:
zorder_map[offset+i].append(o)
return zorder_map


def initialize_dynamic(obj):
"""
Initializes all DynamicMap objects contained by the object
Expand Down Expand Up @@ -311,31 +373,6 @@ def append_refresh(dmap):
return obj.traverse(append_refresh, [DynamicMap])


def get_sources(obj, index=None):
"""
Traverses Callable graph to resolve sources on
DynamicMap objects, returning a list of sources
indexed by the Overlay layer.
"""
layers = [(index, obj)]
if not isinstance(obj, DynamicMap) or not isinstance(obj.callback, Callable):
return layers
index = 0 if index is None else int(index)
for o in obj.callback.inputs:
if isinstance(o, Overlay):
layers.append((None, o))
for i, o in enumerate(overlay):
layers.append((index+i, o))
index += len(o)
elif isinstance(o, DynamicMap):
layers += get_sources(o, index)
index = layers[-1][0]+1
else:
layers.append((index, o))
index += 1
return layers


def traverse_setter(obj, attribute, value):
"""
Traverses the object and sets the supplied attribute on the
Expand Down