Fixed dynamic stream sources assignment in plotting code #1297

philippjfr · 2017-04-16T14:10:29Z

Previously the get_sources function was very wrong in the way it reconstructed the individual layers of an Overlay from a DynamicMap. This resulted in issues in correctly detecting stream sources while plotting. This PR readds the OverlayCallable class which lets an improved get_sources function correctly infer stream sources.

Will add a lot of unit tests for the get_sources function before merging.

philippjfr · 2017-04-16T17:43:09Z

This is ready for review now.

jlstevens · 2017-04-16T19:54:18Z

holoviews/util.py

@@ -80,7 +80,8 @@ def dynamic_operation(*key, **kwargs):
        else:
            def dynamic_operation(*key, **kwargs):
                self.p.kwargs.update(kwargs)
-                _, el = util.get_dynamic_item(map_obj, map_obj.kdims, key)
+                safe_key = () if not map_obj.kdims else key
+                el = map_obj[key]


You could inline map_obj[key] into self._process...

Suggestion still stands.

jlstevens · 2017-04-16T19:55:51Z

holoviews/core/spaces.py

@@ -509,6 +516,47 @@ def get_nested_streams(dmap):
    return list(set(layer_streams))


+
+def get_stream_sources(obj, branch=0):


Any particular reason for the name change from get_sources? Or you just think this is a better name?

Better name, especially in the context of bokeh where I regularly use source to refer to ColumnDataSources.

Sounds reasonable.

What I don't like is that nothing about this mentions streams! It is really get_potential_stream_sources but I'm thinking the name should really reflect that is is about splitting objects in overlays into a dictionary.

I'm thinking this function should really be called something like overlayable_zorder.

jlstevens · 2017-04-16T19:59:29Z

holoviews/core/spaces.py

+            return Dynamic(clone, shared_data=shared_data)
+        elif clone.callback is self.callback:
+            clone.callback = clone.callback.clone()
+        if not any(inp is self for inputs in get_stream_sources(clone).values() for inp in inputs):


How about if all(inp is not self ...?

Why do you only do this if self isn't in the inputs?

So I don't add self to the callback inputs multiple times.

I'll go back to a self not in ... approach for clarity.

This line seems to be the only reason get_stream_sources can't be to plotting.util.

I think you don't need get_stream_sources here as you aren't using z-ordering - you just use .values. If we can make this bit not use get_stream_sources then that function could move to plotting.util.

Agreed, I can make a much simpler function which simply traverses the object, ignoring the zordering.

jlstevens · 2017-04-16T20:03:26Z

holoviews/core/spaces.py

+    """
+    A Callable subclass specifically meant to indicate that the Callable
+    represents an overlay operation between two objects.
+    """


I'm not convinced it is worth introducing a subclass just for isinstance checks. Why not some parameter/attribute on Callable?

This is another classname to know about and it isn't clear from this docstring why callables that involve overlay operations should be special.

Edit: I thought the functions below were methods for a second. They aren't so my point above stands..

It doesn't have methods. I'd be okay making it a parameter on a Callable, but that would make it more likely for users use it incorrectly, which is worse than simply not declaring it at all because it has the potential to mess up the other streams on an Overlay.

Is this a class users should ever make use of directly? I would have no idea whether to use it or not based on the current docstring.

Why not make this an underscore attribute on Callable? Then users have no reason to touch it or know about it?

Sure sounds reasonable.

jlstevens · 2017-04-16T20:07:05Z

holoviews/core/spaces.py

+    Traverses Callable graph to resolve sources on DynamicMap objects,
+    returning a dictionary of sources indexed by the Overlay layer. The
+    branch variables is used for internal record-keeping while recursing
+    through the graph.


I'm still not clear what this means or does. Maybe an example would help?

As this is a function, any chance this could move to util?

Util cannot import from any files in core.

I would recommend plotting.util but that doesn't seem possible either.

jlstevens · 2017-04-16T20:09:46Z

I've had multiple passes through the code and I still have no idea what problem this PR is solving. Could you explain what the problem was?

jlstevens · 2017-04-16T20:13:58Z

holoviews/plotting/bokeh/element.py

-            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]


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?

philippjfr · 2017-04-16T20:17:25Z

Could you explain what the problem was?

When you use dynamic_mul or simply return an Overlay in a DynamicMap, it has to traverse the graph specified by the dynamicmap and assign each node to a particular layer in the returned Overlay. Each subplot created by an OverlayPlot has an index of the layer it is plotting (the zorder), by computing a mapping between the layer index (i.e. zorder) and the sources associated with that layer we can figure out what callbacks a particular plot should create. The previous approach was simply wrong in all kinds of ways.

jlstevens · 2017-04-16T20:43:53Z

holoviews/core/overlay.py

+            callback = OverlayCallable(dynamic_mul, inputs=[self, other])
+            return other.clone(shared_data=False, callback=callback,
+                               streams=[])
+        elif not isinstance(other, ViewableElement):


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.

philippjfr · 2017-04-17T10:20:22Z

@jlstevens This is ready for review again, any suggestions for improving docstrings welcome. The concept of link_inputs will have to be explained in the Linked Streams tutorial.

jlstevens · 2017-04-17T11:06:43Z

holoviews/plotting/util.py

+    return isinstance(obj, DynamicMap) and (isinstance(obj.last, CompositeOverlay))
+
+
+def linked_zorders(obj, path=[]):


Although much better than before, I'm still not sure this name is right. The linked part of the name is about how it is used (candidate sources for streams) not what it does which is better described by the first part of the docstring which talks about 'objects'.

How about object_zorders?

overlayable_zorders?

jlstevens · 2017-04-17T11:07:33Z

holoviews/plotting/util.py

+    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. This is required to determine which
+    subplots should be linked with Stream callbacks.


I would write this on its own line:

'Used to determine which subplots should be linked with Stream callbacks'

jlstevens · 2017-04-17T11:14:50Z

holoviews/core/operation.py

+    link_inputs = param.Boolean(default=False, doc="""
+         Whether a plot created from the output of this operation
+         should be linked to the inputs of this operation.""")
+


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

An operation doesn't create plots, it generates a data-structure.

Specifically, linking is about streams which relates to DynamicMap so this should say something about when the operation is dynamic.

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 ... '

jlstevens · 2017-04-17T11:37:46Z

holoviews/core/operation.py

+         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.""")


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.

jlstevens · 2017-04-17T11:40:40Z

holoviews/core/spaces.py

+    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.


Think this read a bit better:

the link_inputs parameter declares whether the visualization generated using this Callable

jlstevens · 2017-04-17T11:42:09Z

holoviews/core/spaces.py

+         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.""")


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

jlstevens · 2017-04-17T11:42:23Z

holoviews/core/spaces.py

@@ -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


typo: 'areb'

jlstevens · 2017-04-17T11:44:55Z

A general comment about the docstring. For instance you have:

'... determines if linked streams ...'

I would replace all these instance of the word 'if' (when relating to the switch) with the word 'whether'. That might just be my personal preference though...

jlstevens · 2017-04-17T11:54:17Z

holoviews/plotting/util.py

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


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.

jlstevens · 2017-04-17T11:55:08Z

holoviews/plotting/util.py

+    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


Maybe 'which overlaid subplots'?

jlstevens · 2017-04-17T11:56:14Z

holoviews/plotting/util.py

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


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

jlstevens · 2017-04-17T12:03:15Z

holoviews/plotting/util.py

+                zorder_map[0].append(obj)
+        return zorder_map
+
+    isoverlay = isinstance(obj.last, CompositeOverlay)


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...

The relevant nodes are evaluated by this point.

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

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

Right, I agree.

jlstevens · 2017-04-17T12:04:22Z

holoviews/plotting/util.py

+
+    isoverlay = isinstance(obj.last, CompositeOverlay)
+    isdynoverlay = obj.callback._is_overlay
+    found = any(isinstance(p, DynamicMap) and p.callback._overlay for p in path)


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

jlstevens · 2017-04-17T12:05:13Z

holoviews/plotting/util.py

+    isdynoverlay = obj.callback._is_overlay
+    found = any(isinstance(p, DynamicMap) and p.callback._overlay for p in path)
+    if obj not in zorder_map[0] and not isoverlay:
+        zorder_map[0].append(obj)


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

Yes, this is in plotting, the graph has to have been evaluated by this point, even if certain (unimportant) intermediate nodes haven't been.

jlstevens · 2017-04-17T12:08:47Z

holoviews/plotting/util.py

+        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


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

Now handled.

jlstevens · 2017-04-17T12:17:10Z

holoviews/plotting/util.py

        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

        # Recurse into DynamicMap.callback.inputs and update zorder_map
-        i = i if isdynoverlay else 0
+        z = z if isdynoverlay else 0
        dinputs = compute_overlayable_zorders(inp, path=path)


Maybe call this zinputs?

jlstevens · 2017-04-17T12:17:34Z

holoviews/plotting/util.py

        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

        # Recurse into DynamicMap.callback.inputs and update zorder_map
-        i = i if isdynoverlay else 0
+        z = z if isdynoverlay else 0
        dinputs = compute_overlayable_zorders(inp, path=path)
        offset = max(zorder_map.keys())
        for k, v in dinputs.items():


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

jlstevens · 2017-04-17T12:22:40Z

holoviews/plotting/util.py

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

    # 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):
+    for z, 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

        # Recurse into DynamicMap.callback.inputs and update zorder_map


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...

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.

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.

jlstevens · 2017-04-17T14:33:18Z

holoviews/plotting/util.py

+        elif depth is not None and depth < overlay_depth(inp):
+            # Skips branch of graph where the number of elements in an
+            # overlay has been reduced
+            continue


Does this mean linking from inputs is disabled for this case?

Yes, it can't figure out which parts of the input operation should continue to be linked. A user may be able to supply that information but at that point its easier for them to declare their own Callable and list the inputs they need explicitly.

Thanks for clarifying!

For example in the example used for the unit test they could do this:

combined = (area_redim*curve_redim*area2).map(lambda x: x.clone(x.items()[:2]), Overlay) combined.callback.inputs += [area_redim, curve_redim]

That's not bad at and better than trying to complicate things for what I expect is a fairly rare case anyway.

jlstevens · 2017-04-17T14:36:04Z

PR is now looking a lot better than when I first saw it!

The code and concepts involved are fairly tricky but I think it has now been greatly clarified and having lots of unit tests does reassure me. I've asked one more question in a comment but otherwise I am happy to merge once the tests pass.

jlstevens · 2017-04-17T16:03:28Z

Tests are passing.

Merging now so we can test it as much as possible before the release.

philippjfr added 2 commits April 16, 2017 14:57

Readded OverlayCallable subclass to detect dynamic overlays

a283ee7

Fixed get_sources function to detect sources in dynamic overlay

e60d01c

philippjfr added type: bug Something isn't correct or isn't working status: WIP labels Apr 16, 2017

philippjfr added 3 commits April 16, 2017 18:07

Fixes for dynamic_mul logic

1fe4859

Moved and improved get_sources function

9248fb9

Added unit tests to dynamic get_sources function

3ab6adf

philippjfr removed the status: WIP label Apr 16, 2017

Reverted changes to dynamic mul

9cd0109

philippjfr requested a review from jlstevens April 16, 2017 17:42

philippjfr added 2 commits April 16, 2017 19:56

Fixes for deducing stream sources

ce4c476

Small fix for stream sources on batched plots

75d6261

jlstevens reviewed Apr 16, 2017

View reviewed changes

Simplified and improved linked_zorders function

5941c83

philippjfr force-pushed the overlay_source_fix branch from 1423a3a to 5941c83 Compare April 17, 2017 02:05

philippjfr added 2 commits April 17, 2017 10:57

Declare link_inputs on operations and Dynamic utility

1f78b12

Updated docstrings for link_inputs

f88b8f4

jlstevens reviewed Apr 17, 2017

View reviewed changes

Minor docstring edits

243bd41

philippjfr force-pushed the overlay_source_fix branch from a13b916 to 243bd41 Compare April 17, 2017 11:47

jlstevens reviewed Apr 17, 2017

View reviewed changes

Further minor fixes

63c50f6

jlstevens reviewed Apr 17, 2017

View reviewed changes

philippjfr added 2 commits April 17, 2017 15:22

Fixed dynamic overlay bug

945d363

Handle compute_overlayable_zorders edge case

9779aa6

philippjfr force-pushed the overlay_source_fix branch from cf4fef4 to 9779aa6 Compare April 17, 2017 14:25

jlstevens reviewed Apr 17, 2017

View reviewed changes

Fixed small py3 issue

1be4abe

philippjfr force-pushed the overlay_source_fix branch from 2c3f6d4 to 1be4abe Compare April 17, 2017 15:06

jlstevens merged commit 8d6be58 into master Apr 17, 2017

This was referenced Apr 17, 2017

Handle additional linked stream condition #1303

Merged

Reverted change to Dynamic utility #1304

Closed

philippjfr deleted the overlay_source_fix branch April 19, 2017 21:38

		@@ -509,6 +516,47 @@ def get_nested_streams(dmap):
		return list(set(layer_streams))



		def get_stream_sources(obj, branch=0):

		return isinstance(obj, DynamicMap) and (isinstance(obj.last, CompositeOverlay))


		def linked_zorders(obj, path=[]):

Fixed dynamic stream sources assignment in plotting code #1297

Fixed dynamic stream sources assignment in plotting code #1297

Conversation

philippjfr commented Apr 16, 2017

philippjfr commented Apr 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Apr 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens commented Apr 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippjfr commented Apr 16, 2017

Choose a reason for hiding this comment

philippjfr commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

jlstevens Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens commented Apr 17, 2017

jlstevens Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippjfr Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens commented Apr 17, 2017

jlstevens commented Apr 17, 2017

jlstevens Apr 16, 2017 •

edited

Loading

jlstevens Apr 17, 2017 •

edited

Loading

jlstevens Apr 17, 2017 •

edited

Loading

jlstevens Apr 17, 2017 •

edited

Loading

jlstevens Apr 17, 2017 •

edited

Loading

philippjfr Apr 17, 2017 •

edited

Loading