-
-
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
Fixed dynamic stream sources assignment in plotting code #1297
Changes from 12 commits
a283ee7
e60d01c
1fe4859
9248fb9
3ab6adf
9cd0109
ce4c476
75d6261
5941c83
1f78b12
f88b8f4
048642f
521f7be
243bd41
63c50f6
945d363
9779aa6
1be4abe
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 |
---|---|---|
|
@@ -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.""") | ||
|
||
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. Needs a lot more explanation as almost none of the required concepts exist at this level:
As a very rough suggestion, it should be something along the lines of:
|
||
streams = param.List(default=[], doc=""" | ||
List of streams that are applied if dynamic=True, allowing | ||
for dynamic interaction with the plot.""") | ||
|
@@ -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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
|
@@ -41,7 +42,6 @@ def dynamic_mul(*args, **kwargs): | |
|
||
|
||
|
||
|
||
class CompositeOverlay(ViewableElement, Composable): | ||
""" | ||
CompositeOverlay provides a common baseclass for Overlay classes. | ||
|
@@ -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): | ||
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 really need to refactor |
||
raise NotImplementedError | ||
return Overlay.from_values([self, other]) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,11 +117,14 @@ 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.kdims: | ||
self_el = self.select(**key_map) | ||
else: | ||
self_el = self[()] | ||
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. Might as well be: self_el = self.select(**key_map) if self.kdims else self[()] |
||
if self_el is not None: | ||
layers.append(self_el) | ||
else: | ||
|
@@ -130,8 +133,10 @@ def dynamic_mul(*key, **kwargs): | |
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.kdims: | ||
other_el = other.select(**key_map) | ||
else: | ||
other_el = other[()] | ||
if other_el is not None: | ||
layers.append(other_el) | ||
else: | ||
|
@@ -140,6 +145,7 @@ def dynamic_mul(*key, **kwargs): | |
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=[]) | ||
|
@@ -207,6 +213,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()] | ||
|
@@ -413,7 +420,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. | ||
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. Think this read a bit better:
|
||
|
||
The mapping should map from an appropriate key to a list of | ||
streams associated with the selected object. The appropriate key | ||
|
@@ -429,6 +440,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 | ||
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. 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.""") | ||
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. 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 | ||
|
@@ -441,6 +462,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): | ||
|
@@ -687,10 +710,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 | ||
|
||
|
||
|
@@ -1104,7 +1124,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: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
""" | ||
|
@@ -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.""") | ||
|
||
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. Same issue as for the shade operation. |
||
@classmethod | ||
def uint8_to_uint32(cls, img): | ||
shape = img.shape | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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] | ||
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. 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? 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. See below |
||
else: | ||
sources = [(self.zorder, self.hmap.last)] | ||
cb_classes = set() | ||
|
@@ -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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
from __future__ import unicode_literals | ||
from collections import defaultdict | ||
|
||
import numpy as np | ||
import param | ||
|
@@ -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): | ||
|
@@ -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 | ||
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. 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:
|
||
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 | ||
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. 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): | ||
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. Might be more readable renaming the |
||
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) | ||
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. 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 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 relevant nodes are evaluated by this point. 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. Ok, just to be sure I would assert that 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. No, not all paths have to have been evaluated, that would break everything. 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. Right, I agree. |
||
isdynoverlay = obj.callback._is_overlay | ||
found = any(isinstance(p, DynamicMap) and p.callback._overlay for p in path) | ||
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. I think the definition of |
||
if obj not in zorder_map[0] and not isoverlay: | ||
zorder_map[0].append(obj) | ||
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. I think this would execute if 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. 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 | ||
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. What happens if you have an operation that takes in an overlay of 3 things and returns an overlay of 2 things? 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. Now handled. |
||
|
||
# Recurse into DynamicMap.callback.inputs and update zorder_map | ||
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. So this would not happen for a DynamicMap callback with inputs that is user defined i.e 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.
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. 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. 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 Seems fine. |
||
i = i if isdynoverlay else 0 | ||
dinputs = compute_overlayable_zorders(inp, path=path) | ||
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. Maybe call this |
||
offset = max(zorder_map.keys()) | ||
for k, v in dinputs.items(): | ||
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. Instead of |
||
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 | ||
|
@@ -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 | ||
|
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.
Much better!
Minor edits: