-
-
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
Conversation
This is ready for review now. |
holoviews/util.py
Outdated
@@ -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] |
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.
You could inline map_obj[key]
into self._process
...
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.
Suggestion still stands.
holoviews/core/spaces.py
Outdated
@@ -509,6 +516,47 @@ def get_nested_streams(dmap): | |||
return list(set(layer_streams)) | |||
|
|||
|
|||
|
|||
def get_stream_sources(obj, branch=0): |
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.
Any particular reason for the name change from get_sources
? Or you just think this is a better name?
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.
Better name, especially in the context of bokeh where I regularly use source
to refer to ColumnDataSources.
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.
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.
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.
I'm thinking this function should really be called something like overlayable_zorder
.
holoviews/core/spaces.py
Outdated
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): |
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.
How about if all(inp is not self ...
?
Why do you only do this if self isn't in the inputs?
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.
So I don't add self to the callback inputs multiple times.
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.
I'll go back to a self not in ...
approach for clarity.
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.
This line seems to be the only reason get_stream_sources
can't be to plotting.util
.
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.
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
.
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.
Agreed, I can make a much simpler function which simply traverses the object, ignoring the zordering.
holoviews/core/spaces.py
Outdated
""" | ||
A Callable subclass specifically meant to indicate that the Callable | ||
represents an overlay operation between two objects. | ||
""" |
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.
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.
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.
Edit: I thought the functions below were methods for a second. They aren't so my point above stands..
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.
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.
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.
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.
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.
Why not make this an underscore attribute on Callable
? Then users have no reason to touch it or know about it?
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.
Sure sounds reasonable.
holoviews/core/spaces.py
Outdated
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. |
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.
I'm still not clear what this means or does. Maybe an example would help?
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.
As this is a function, any chance this could move to util?
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.
Util cannot import from any files in core.
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.
I would recommend plotting.util
but that doesn't seem possible either.
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] |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
See below
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. |
holoviews/core/overlay.py
Outdated
callback = OverlayCallable(dynamic_mul, inputs=[self, other]) | ||
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 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.
1423a3a
to
5941c83
Compare
@jlstevens This is ready for review again, any suggestions for improving docstrings welcome. The concept of |
holoviews/plotting/util.py
Outdated
return isinstance(obj, DynamicMap) and (isinstance(obj.last, CompositeOverlay)) | ||
|
||
|
||
def linked_zorders(obj, path=[]): |
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.
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
?
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.
overlayable_zorders
?
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.
Sure.
holoviews/plotting/util.py
Outdated
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. |
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.
I would write this on its own line:
'Used to determine which subplots should be linked with Stream callbacks'
holoviews/core/operation.py
Outdated
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.""") | ||
|
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.
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 ... '
holoviews/core/spaces.py
Outdated
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 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.
holoviews/core/spaces.py
Outdated
@@ -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 |
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.
typo: 'areb'
A general comment about the docstring. For instance you have:
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... |
a13b916
to
243bd41
Compare
holoviews/plotting/util.py
Outdated
|
||
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 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.
holoviews/plotting/util.py
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 'which overlaid subplots'?
holoviews/plotting/util.py
Outdated
# 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 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[0].append(obj) | ||
return zorder_map | ||
|
||
isoverlay = isinstance(obj.last, CompositeOverlay) |
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.
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...
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.
The relevant nodes are evaluated by this point.
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.
Ok, just to be sure I would assert that .last
is not None
to make sure initial_key
was called.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I agree.
holoviews/plotting/util.py
Outdated
|
||
isoverlay = isinstance(obj.last, CompositeOverlay) | ||
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 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.
holoviews/plotting/util.py
Outdated
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) |
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.
I think this would execute if .last
is None. Maybe this is ok because you are assuming initial_key
is called already?
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.
Yes, this is in plotting, the graph has to have been evaluated by this point, even if certain (unimportant) intermediate nodes haven't been.
holoviews/plotting/util.py
Outdated
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 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?
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.
Now handled.
holoviews/plotting/util.py
Outdated
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) |
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.
Maybe call this zinputs
?
holoviews/plotting/util.py
Outdated
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(): |
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.
Instead of k
maybe this should be called zinp
or something?
holoviews/plotting/util.py
Outdated
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 |
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.
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...
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.
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.
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.
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.
cf4fef4
to
9779aa6
Compare
holoviews/plotting/util.py
Outdated
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 |
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.
Does this mean linking from inputs is disabled for this case?
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.
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.
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.
Thanks for clarifying!
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.
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]
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.
That's not bad at and better than trying to complicate things for what I expect is a fairly rare case anyway.
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. |
2c3f6d4
to
1be4abe
Compare
Tests are passing. Merging now so we can test it as much as possible before the release. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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 improvedget_sources
function correctly infer stream sources.Will add a lot of unit tests for the
get_sources
function before merging.