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

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Apr 16, 2017

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 philippjfr removed the WIP label Apr 16, 2017

@philippjfr philippjfr requested a review from jlstevens Apr 16, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 16, 2017

This is ready for review now.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Suggestion still stands.

@@ -509,6 +516,47 @@ def get_nested_streams(dmap):
return list(set(layer_streams))
def get_stream_sources(obj, branch=0):

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 16, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

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):

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

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

This comment has been minimized.

@philippjfr

philippjfr Apr 16, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 16, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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.

This comment has been minimized.

@philippjfr

philippjfr Apr 16, 2017

Member

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 16, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 16, 2017

Member

Sure sounds reasonable.

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 16, 2017

Member

Util cannot import from any files in core.

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 16, 2017

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?

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]

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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?

This comment has been minimized.

@philippjfr

philippjfr Apr 16, 2017

Member

See below

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 16, 2017

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.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 16, 2017

Member

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

This comment has been minimized.

Member

philippjfr commented Apr 17, 2017

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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?

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2017

Member

overlayable_zorders?

This comment has been minimized.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

I would write this on its own line:

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

@@ -429,6 +439,10 @@ 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="""
Whether a plot created from this Callable should be linked
to the declared inputs.""")

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Similar issue with the link_inputs parameter on operations - it needs to explain a lot more but in terms of Callable concepts this time.

@jlstevens

I'm submitting this review to cancel it. I accidentally hit the review button and want to go back to individual comments.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Same issue as for the shade operation.

if self.kdims:
self_el = self.select(**key_map)
else:
self_el = self[()]

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Might as well be:

self_el = self.select(**key_map) if self.kdims else self[()]
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.""")

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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.

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Think this read a bit better:

the link_inputs parameter declares whether the visualization generated using this 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.""")

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

typo: 'areb'

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 17, 2017

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Maybe 'which overlaid subplots'?

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2017

Member

The relevant nodes are evaluated by this point.

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Right, I agree.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

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)

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2017

Member

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Now handled.

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)

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Maybe call this zinputs?

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():

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

Thanks for clarifying!

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2017

Member

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]

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2017

Member

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

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 17, 2017

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

This comment has been minimized.

Member

jlstevens commented Apr 17, 2017

Tests are passing.

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

@jlstevens jlstevens merged commit 8d6be58 into master Apr 17, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 78.927%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the overlay_source_fix branch Apr 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment