Skip to content
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

Dynamic collation #1243

Merged
merged 13 commits into from Apr 5, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

philippjfr commented Apr 4, 2017

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

This comment has been minimized.

Copy link
Contributor

jbednar commented Apr 4, 2017

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.

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.

This comment has been minimized.

@jbednar

jbednar Apr 4, 2017

Contributor

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?

This comment has been minimized.

@philippjfr

philippjfr Apr 4, 2017

Author Contributor

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

This comment has been minimized.

@jbednar

jbednar Apr 4, 2017

Contributor

Viewable?

This comment has been minimized.

@jbednar

jbednar Apr 4, 2017

Contributor

View? Can't remember.

This comment has been minimized.

@jlstevens

jlstevens Apr 4, 2017

Contributor

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

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Apr 4, 2017

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

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Apr 4, 2017

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

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Apr 4, 2017

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

This comment has been minimized.

Copy link
Contributor

jlstevens commented Apr 4, 2017

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

This comment has been minimized.

Copy link
Contributor

jbednar commented Apr 4, 2017

Sounds good to me.

@philippjfr philippjfr force-pushed the dynamic_collation branch from 2c5372c to a2cfa4c Apr 5, 2017

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Apr 5, 2017

Ready to review/merge.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Apr 5, 2017

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.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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)

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 comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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?

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

Agreed

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

Makes sense as a utility. Thanks.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

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)

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

Ok, that makes sense.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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?

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

Hmm, then using it explicitly is a bit awkward.

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

Why warn?

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

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

Sure.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

Sounds good.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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.

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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.

This comment has been minimized.

@jlstevens

jlstevens Apr 5, 2017

Contributor

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

This comment has been minimized.

Copy link
Contributor

jlstevens commented Apr 5, 2017

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

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Apr 5, 2017

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

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Apr 5, 2017

Great! Tests are passing now.

Merging.

@jlstevens jlstevens merged commit ea22ec8 into master Apr 5, 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.418%
Details
s3-reference-data-cache Test data is cached.
Details

@jbednar jbednar deleted the dynamic_collation branch Apr 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.