Dynamic collation #1243

philippjfr · 2017-04-04T14:59:37Z

Implements dynamic collation as requested and described in #1188. The user can declare a stream on a DynamicMap returning a (Nd)Layout, and then specify which item in the Layout should be used as the source for the stream in the collate call by either supplying a list of streams for each item or a dictionary where the key corresponds to the index of the item in the Layout. Here's a small example:

%%opts Layout [shared_axes=False]

def test(x_range, y_range):
    return hv.Image(np.random.rand(10,10)) + hv.Bounds((x_range[0], y_range[0], x_range[1], y_range[1]))

stream = hv.streams.RangeXY(x_range=(0,-.5), y_range=(0,-0.5))
dmap = hv.DynamicMap(test, streams=[stream], kdims=[])

dmap.collate(streams={0: [stream]})
# OR dmap.collate(streams=[[stream]])

Here the RangeXY stream will be attached to the Image and the Bounds in the second subplot will reflect the zoom ranges of the first plot. Leaving out the streams declaration on the collate call will mean that the Stream gets ranges from both subplots.

jbednar · 2017-04-04T16:17:30Z

Thanks so much! This will be really useful functionality.

Integer indexing is easy enough to specify, but it seems fragile and also difficult to understand when skimming (the reader won't immediately see what "0" might mean). I had to re-read your description several times before I was fairly confident I understood how it was meant to work. Is there a way to specify the item by type.group.label-style paths as well, to make it more explicit what is connected to what? In this case there's no explicit label for any of the plots, but maybe if there were, it would be easier for people to see which bit is connected to what.

jbednar · 2017-04-04T16:21:14Z

holoviews/core/spaces.py

+        this to work correctly. In order to attach a stream as a source
+        for a particular object in the Layout you may supply either
+        a dictionary or list of lists of streams corresponding to each
+        Element in the Layout.


Layouts don't just contain Elements, do they? E.g. an Overlay isn't an Element, is it, yet can be contained in a Layout?

Yes, need to be clear about terminology here. Not quite clear what the correct term is anymore.

View? Can't remember.

I think Viewable may be right. We rarely use that term though but I don't think I mind it here...

philippjfr · 2017-04-04T16:27:51Z

Is there a way to specify the item by group.label-style paths as well, to make it more explicit what is connected to what?

Sure, I can add that, personally that seems more painful to specify but there's no reason why it shouldn't be allowed.

philippjfr · 2017-04-04T16:43:45Z

@jbednar Would you expect only the full type[.group][.label] spec to match or should it allow partial matching?

jbednar · 2017-04-04T18:14:42Z

I'd expect whatever normally works elsewhere to work in this case. Options are probably the most common place people encounter such paths, and they do support partial matching. Moreover, I do think partial matching would be useful here (e.g. if I have an Image next to a Curve, it would be very clear to say I want this stream to be attached to the Image plot, not the Curve plot). Plus getting a partial match is a lot easier than getting a complete match, given that labels are often auto-generated. So yes, I would like partial matching, but since you're the one doing it, you'd be the one to make the call. :-) Thanks.

jlstevens · 2017-04-04T19:15:18Z

I think getting collate to support DynamicMaps that output layouts is a great idea!

That said, I am strongly in favor of supplying the association between streams and elements up front so that you can just call collate() (without arguments) and have it work.

For this reason, I would prefer something like:

def test(x_range, y_range):
    return hv.Image(np.random.rand(10,10)) + hv.Bounds((x_range[0], y_range[0], x_range[1], y_range[1]))

stream = hv.streams.RangeXY(x_range=(0,-.5), y_range=(0,-0.5))
dmap = hv.DynamicMap(Callable(test, stream_mapping=spec), streams=[stream], kdims=[])

Where stream_mapping is a similar type of specification as you were implementing collate. Then if the Callable returns a Layout and this specification is defined, collate can be called automatically. I really think it is best to declare the stream associations up-front when defining the DynamicMap (and not afterwards).

jbednar · 2017-04-04T19:27:16Z

Sounds good to me.

philippjfr · 2017-04-05T16:44:08Z

Ready to review/merge.

jlstevens · 2017-04-05T19:03:22Z

Reviewing it now. One initial comment though..

Do you have what you consider a 'nice' (simple) example to show us what the code looks like, with the corresponding output?

I know the unit tests has lots of examples but it would be good to have one you would like to highlight. It would then be something I would consider incorporating into the updated DynamicMap tutorials when I get round to working on those docs.

jlstevens · 2017-04-05T19:11:58Z

holoviews/core/spaces.py

+    callbacks which return composite objects like (Nd)Layout and
+    GridSpace objects. The mapping should map between an integer index
+    or a type[.group][.label] specification and lists of streams
+    matching the object.
    """


Some of this is very hard to parse. Here is my attempt at making it easier to read:

A Callable may also specify a stream_mapping which specifies the objects that are associated with interactive (i.e linked) streams 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.

The mapping should map from an appropriate key to a list of streams associated with the selected object. The appropriate key may be a type[.group][.label] specification for Layouts, an integer index or a suitable NdLayout/GridSpace key. For more information see the DynamicMap tutorial at holoviews.org.

(Assuming it would be the DynamicMap tutorial)

jlstevens · 2017-04-05T19:12:55Z

holoviews/core/spaces.py

+    callbacks which return composite objects like (Nd)Layout and
+    GridSpace objects. The mapping should map between an integer index
+    or a type[.group][.label] specification and lists of streams
+    matching the object.
    """

    callable_function = param.Callable(default=lambda x: x, doc="""


This doesn't have to happen in this PR but I'm strongly considering having this renamed to simply callable. It would be a parameter/attribute and so wouldn't shadow the callable builtin.

jlstevens · 2017-04-05T19:15:34Z

holoviews/core/spaces.py

-    def __init__(self, **params):
+    def __init__(self, callable_function=None, stream_mapping={}, **params):
+        if callable_function is not None:
+            params['callable_function'] = callable_function


Any reason stream_mapping can't just be a parameter, allowing us to go back to the def __init__(self, **params) constructor?

In addition, how can callable_function be None? Isn't it required to define a valid Callable?

Any reason stream_mapping can't just be a parameter, allowing us to go back to the def init(self, **params) constructor?

Agreed, not sure why I didn't make it one.

In addition, how can callable_function be None? Isn't it required to define a valid Callable?

The parameter currently defines a no-op, which this leaves unchanged, but thinking about it, I think callable should just be None on the parameter and become a required first argument.

I don't think callable should just be None and become a required first argument.

Agreed

jlstevens · 2017-04-05T19:17:18Z

holoviews/core/spaces.py

+                undefined.append(kdim)
+        if undefined:
+            raise KeyError('dimensions do not specify a range or values, '
+                           'cannot supply initial key' % ', '.join(undefined))


No harm having this check here but I was thinking that DynamicMap should also be doing similar (if not identical) validation. I definitely want to be checking that kdims have the appropriate range settings there.

I think we already assert that when you're not in sampled mode.

jlstevens · 2017-04-05T19:23:37Z

holoviews/core/spaces.py

+        correctly. Associating streams with specific viewables in the
+        returned container declare a stream_mapping on the DynamicMap
+        Callable during instantiation.
+        """


Collation allows collapsing DynamicMaps ...

'restructuring'? I'm not sure it is necessarily 'collapsing'...

Collating will split the DynamicMap into of individual DynamicMaps.

Probably mostly true but what if the callback returns something invalid lower down in the hierarchy that (Nd)Layout or GridSpace? If collate were entirely general it would fix other weird things like NdOverlays of Overlays (actually what happens right now if you try this?).

'Note that the composite object ...' I would add the word 'returned' as in 'Note that the returned composite object ...'

Associating streams with specific viewables in the returned container declare a stream_mapping on the DynamicMap Callable during instantiation.

Probably information that is more useful as a comment for developers and users. Users should be guided by the appropriate warning earlier on if they need to use stream_mapping so collate probably doesn't need to mention it (it should all just work!).

jlstevens · 2017-04-05T19:24:35Z

holoviews/plotting/renderer.py

-                # Skip initialization until plotting code
-                continue
-            dmap[dmap._initial_key()]
+            initialize_dynamic(obj)


Makes sense as a utility. Thanks.

jlstevens · 2017-04-05T19:26:42Z

holoviews/plotting/util.py

+    dmaps = obj.traverse(lambda x: x, specs=[DynamicMap])
+    for dmap in dmaps:
+        if dmap.sampled:
+            # Skip initialization until plotting code


Does this mean plotting does some of its own initialization? Maybe this utility could have a samples argument (or similar) to handle that case as well i.e if samples is None, this function stays the same as it is now, otherwise handles what is needed for sampled DynamicMaps.

I don't think there is any explicit initialization in the plotting code it just works because select is called requesting the first sample defined by other HoloMap(s).

Ok, that makes sense. No need to change this utility then.

jlstevens · 2017-04-05T19:29:05Z

holoviews/streams.py

-            raise Exception('source has already been defined on stream.')
+            source_list = self.registry[id(self._source)]
+            if self in source_list:
+                source_list.remove(self)


Looks like it didn't clear itself up before...surely that causes some bugs?

Overriding an existing source raised an Exception before, but I think it's fine as it's not something you'd do without knowing what you're doing anyway.

Ok, that makes sense.

jlstevens · 2017-04-05T19:40:17Z

holoviews/core/spaces.py

+        if self.last is not None:
+            pass
+        else:
+            self[self._initial_key()]


Docstring should mention that collate will initialize the dynamicmap if it hasn't been already.

Alternatively, shouldn't collate just complain if the dynamicmap is uninitialized and the renderer can initialize?

Hmm, then using it explicitly is a bit awkward.

How about an initialize argument defaults to True? I don't object to this as it is a simple boolean that disables behavior that is normally helpful (but may be unexpected).

Hmm, and raise an Exception when False? Doesn't seem very useful.

I'm not saying this option would be used a lot (in fact I'm counting on the opposite). I just think we can offer the option to disable some slightly surprising behavior. Won't calling self[self._initial_key()] change the cache? It must do as you are then using last.

Maybe you should clone self and then use self[self._initial_key()] which would then not result in a side-effect on the DynamicMap that collate is called on. Then, if I understand correctly, the use of .last is just to get an example of the sort of thing the callback is returning...

If that makes sense, there would be no harm in using initial_key internally.

Making a clone as to not pollute the cache seems fine, an option that gives you an exception when you use it doesn't seem sensible.

Agreed. The reason I suggest the clone is so we don't need to worry about adding an option.

jlstevens · 2017-04-05T19:42:48Z

holoviews/core/spaces.py

+            return new_item
+        else:
+            self.warning('DynamicMap does not need to be collated.')
+            return dmap


Why warn?

It isn't inefficient to call collate pointlessly in such a case so just return the DynamicMap as it is already ok.

jlstevens · 2017-04-05T19:46:22Z

holoviews/core/spaces.py

+        elif isinstance(self.last, (Layout, NdLayout, GridSpace)):
+            # Expand Layout/NdLayout
+            from ..util import Dynamic
+            new_item = self.last.clone(shared_data=False)


Based on the isinstance check, container might be a better name for this variable than new_item...

Sounds good.

jlstevens · 2017-04-05T19:49:24Z

holoviews/core/spaces.py

+                        "The stream_mapping supplied on the Callable "
+                        "is ambiguous please supply more specific Layout "
+                        "path specs.")
+                remapped_streams += vstreams


Maybe this could be a remap_streams utility?

As far as I can tell the required signature would be remap_streams(stream_mapping, object) where object here is self.last.

Edit: The signature might have to be remap_streams(stream_mapping, object, in_layout) where object is in the loop and it returns vstreams

I can see lines 858-867 becoming a nice self-contained utility, up to you if you think it's worth it.

I think it is worth it if it can be given a sensible description in the docstring that makes sense and if you would like to write a few unit tests to both test it and show how it is used.

jlstevens · 2017-04-05T19:56:52Z

holoviews/core/spaces.py

+                def collation_cb(*args, **kwargs):
+                    return self[args][kwargs['collation_key']]
+                callback = Callable(partial(collation_cb, collation_key=k),
+                                    inputs=[self])


A couple of question:

Why use a partial? Can't collation_cb use k directly, forming a closure? Or is the issue that k is changing in a loop?

To make sure I understand, collation_key is really about selecting a portion of the structure returned by the callback? In which case selection_key might be a more intuitive name...

Why use a partial? Can't collation_cb use k directly, forming a closure? Or is the issue that k is changing in a loop?

Yes.

To make sure I understand, collation_key is really about selecting a portion of the structure returned by the callback? In which case selection_key might be a more intuitive name...

Sure.

jlstevens · 2017-04-05T20:04:30Z

holoviews/core/spaces.py

+                # Remap source of streams
+                for stream in vstreams:
+                    if stream.source is self:
+                        stream.source = vdmap


Is this needed? Won't clone go through the DynamicMap constructor which assigns the stream sources to self?

Not if it's already set to the original DynamicMap (which it is guaranteed to be).

Ah, you are only doing this if stream.source is self (instance collate is called on). Not sure why yet...

This is the what does the actual remapping. You're remapping from the original DynamicMap returning the Layout to the DynamicMap created by collate, which returns a specific item.

Got it. And you need the full vstreams even if they don't have source as self as you should supply all streams no matter their source.

jlstevens · 2017-04-05T20:08:38Z

I've made a full pass and posted my immediate comments. Generally looks good and my suggestions are mostly for clarification, not for any major refactoring.

philippjfr · 2017-04-05T20:51:02Z

Made all suggested fixes, except for creating another utility, which I don't think is worth it.

jlstevens · 2017-04-05T22:01:41Z

Great! Tests are passing now.

Merging.

github-actions · 2024-10-25T23:28:16Z

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.

philippjfr added tag: API type: feature A major new feature labels Apr 4, 2017

jbednar reviewed Apr 4, 2017

View reviewed changes

philippjfr added 7 commits April 5, 2017 03:15

Allowed overriding a Stream source

e5c246a

Made callable_function positional arg of Callable

abee6be

Raise Exception about uninitializable DynamicMap

b1ca5a1

Implemented collation for nested DynamicMaps

756f1a2

Handled automatic collation of DynamicMaps

2d09647

Small fix for dynamic collation closure

002ded4

Moved stream_remapping specification onto Callable

a2cfa4c

philippjfr force-pushed the dynamic_collation branch from 2c5372c to a2cfa4c Compare April 5, 2017 02:29

philippjfr added 3 commits April 5, 2017 12:09

Updated DynamicMap.collate docstring

cde0972

Added validation for ambiguous stream mapping

fab70ee

Added tests for dynamic collation

6f4d956

jlstevens reviewed Apr 5, 2017

View reviewed changes

philippjfr added 2 commits April 5, 2017 21:46

Cleaned up Callable renaming callable_function

91e881e

Simplified DynamicMap.collate

5196ddc

Fixed initialization bug on dynamic collate

00bec26

jlstevens merged commit ea22ec8 into master Apr 5, 2017

jbednar deleted the dynamic_collation branch April 5, 2017 22:56

github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024

Dynamic collation #1243

Dynamic collation #1243

Conversation

philippjfr commented Apr 4, 2017

jbednar commented Apr 4, 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

philippjfr commented Apr 4, 2017

philippjfr commented Apr 4, 2017

jbednar commented Apr 4, 2017 • edited Loading

jlstevens commented Apr 4, 2017 • edited Loading

jbednar commented Apr 4, 2017

philippjfr commented Apr 5, 2017

jlstevens commented Apr 5, 2017

jlstevens Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippjfr Apr 5, 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

jlstevens Apr 5, 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

jlstevens Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Apr 5, 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

jlstevens Apr 5, 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

philippjfr Apr 5, 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 5, 2017

philippjfr commented Apr 5, 2017

jlstevens commented Apr 5, 2017

github-actions bot commented Oct 25, 2024

jbednar commented Apr 4, 2017 •

edited

Loading

jbednar commented Apr 4, 2017 •

edited

Loading

jlstevens commented Apr 4, 2017 •

edited

Loading

jlstevens Apr 5, 2017 •

edited

Loading

philippjfr Apr 5, 2017 •

edited

Loading

jlstevens Apr 5, 2017 •

edited

Loading

jlstevens Apr 5, 2017 •

edited

Loading

jlstevens Apr 5, 2017 •

edited

Loading

jlstevens Apr 5, 2017 •

edited

Loading

jlstevens Apr 5, 2017 •

edited

Loading

philippjfr Apr 5, 2017 •

edited

Loading