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
Copy path View file
@@ -402,6 +402,13 @@ class Callable(param.Parameterized):
returned value based on the arguments to the function and the
state of all streams on its inputs, to avoid calling the function
unnecessarily.
A Callable may also specify a stream_mapping which allows
specifying which objects to attached linked streams to on
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.

Copy link
@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)


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

This comment has been minimized.

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

@@ -410,9 +417,12 @@ class Callable(param.Parameterized):
inputs = param.List(default=[], doc="""
The list of inputs the callable function is wrapping.""")

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.

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

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

Copy link
@jlstevens

jlstevens Apr 5, 2017

Contributor

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

Agreed

super(Callable, self).__init__(**params)
self._memoized = {}
self.stream_mapping = stream_mapping

def __call__(self, *args, **kwargs):
inputs = [i for i in self.inputs if isinstance(i, DynamicMap)]
@@ -496,11 +506,17 @@ def _initial_key(self):
values on the key dimensions.
"""
key = []
undefined = []
for kdim in self.kdims:
if kdim.values:
key.append(kdim.values[0])
elif kdim.range:
key.append(kdim.range[0])
else:
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.

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

Copy link
@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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

return tuple(key)


@@ -788,6 +804,105 @@ def dynamic_redim(obj):
return Dynamic(redimmed, shared_data=True, operation=dynamic_redim)


def collate(self):
"""
Collation allows collapsing DynamicMaps with invalid nesting
hierarchies. This is particularly useful when defining
DynamicMaps returning an (Nd)Layout or GridSpace
type. Collating will split the DynamicMap into of individual
DynamicMaps. Note that the composite object has to be of
consistent length and types for this to work
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.

Copy link
@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!).

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

This comment has been minimized.

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

Copy link
@philippjfr

philippjfr Apr 5, 2017

Author Contributor

Hmm, then using it explicitly is a bit awkward.

This comment has been minimized.

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

Copy link
@philippjfr

philippjfr Apr 5, 2017

Author Contributor

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

This comment has been minimized.

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

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

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


if isinstance(self.last, HoloMap):
# Get nested kdims and streams
streams = list(self.streams)
if isinstance(self.last, DynamicMap):
dimensions = [d(values=self.last.dimension_values(d.name))
for d in self.last.kdims]
streams += self.last.streams
stream_kwargs = set()
for stream in streams:
contents = set(stream.contents())
if stream_kwargs & contents:
raise KeyError('Cannot collate DynamicMaps with clashing '
'stream parameters.')
else:
dimensions = self.last.kdims
kdims = self.kdims+dimensions

# Define callback
def collation_cb(*args, **kwargs):
return self[args[:self.ndims]][args[self.ndims:]]
callback = Callable(collation_cb, inputs=[self])

return self.clone(shared_data=False, callback=callback,
kdims=kdims, streams=streams)
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.

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

Copy link
@philippjfr

philippjfr Apr 5, 2017

Author Contributor

Sounds good.


# Get stream mapping from callback
remapped_streams = []
streams = self.callback.stream_mapping
for i, (k, v) in enumerate(self.last.data.items()):
vstreams = streams.get(i, [])
if not vstreams:
if isinstance(self.last, Layout):
for l in range(len(k)):
path = '.'.join(k[:l])
if path in streams:
vstreams = streams[path]
break
else:
vstreams = streams.get(k, [])
if any(s in remapped_streams for s in vstreams):
raise ValueError(
"The stream_mapping supplied on the Callable "
"is ambiguous please supply more specific Layout "
"path specs.")
remapped_streams += vstreams

This comment has been minimized.

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

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

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


# Define collation callback
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.

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

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

vdmap = self.clone(callback=callback, shared_data=False,
streams=vstreams)

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

This comment has been minimized.

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

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

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

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

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

new_item[k] = vdmap

unmapped_streams = [repr(stream) for stream in self.streams
if (stream.source is self) and
(stream not in remapped_streams)
and stream.linked]
if unmapped_streams:
raise ValueError(
'The following streams are set to be automatically '
'linked to a plot, but no stream_mapping specifying '
'which item in the (Nd)Layout to link it to was found:\n%s'
% ', '.join(unmapped_streams)
)
return new_item
else:
self.warning('DynamicMap does not need to be collated.')
return dmap

This comment has been minimized.

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

Copy link
@philippjfr

philippjfr Apr 5, 2017

Author Contributor

Sure.



def groupby(self, dimensions=None, container_type=None, group_type=None, **kwargs):
"""
Implements a dynamic version of a groupby, which will
Copy path View file
@@ -18,7 +18,7 @@
from .. import DynamicMap
from . import Plot
from .comms import JupyterComm
from .util import displayable, collate
from .util import displayable, collate, initialize_dynamic

from param.parameterized import bothmethod

@@ -159,16 +159,12 @@ def get_plot(self_or_cls, obj, renderer=None):
"""
Given a HoloViews Viewable return a corresponding plot instance.
"""
# Initialize DynamicMaps with first data item
initialize_dynamic(obj)

if not isinstance(obj, Plot) and not displayable(obj):
obj = collate(obj)

# Initialize DynamicMaps with first data item
dmaps = obj.traverse(lambda x: x, specs=[DynamicMap])
for dmap in dmaps:
if dmap.sampled:
# Skip initialization until plotting code
continue
dmap[dmap._initial_key()]
initialize_dynamic(obj)

This comment has been minimized.

Copy link
@jlstevens

jlstevens Apr 5, 2017

Contributor

Makes sense as a utility. Thanks.


if not renderer: renderer = self_or_cls.instance()
if not isinstance(obj, Plot):
Copy path View file
@@ -42,13 +42,15 @@ def collate(obj):
"structure shown in the Composing Data tutorial"
"(http://git.io/vtIQh)" % nested_type)
return obj.collate()
if isinstance(obj, DynamicMap):
return obj.collate()
if isinstance(obj, HoloMap):
display_warning.warning("Nesting %ss within a HoloMap makes it difficult "
display_warning.warning("Nesting {0}s within a {1} makes it difficult "
"to access your data or control how it appears; "
"we recommend calling .collate() on the HoloMap "
"we recommend calling .collate() on the {1} "
"in order to follow the recommended nesting "
"structure shown in the Composing Data tutorial"
"(http://git.io/vtIQh)" % obj.type.__name__)
"(http://git.io/vtIQh)".format(obj.type.__name__, type(obj).__name__))
return obj.collate()
elif isinstance(obj, (Layout, NdLayout)):
try:
@@ -70,6 +72,19 @@ def collate(obj):
raise Exception(undisplayable_info(obj))


def initialize_dynamic(obj):
"""
Initializes all DynamicMap objects contained by the object
"""
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.

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

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

Copy link
@jlstevens

jlstevens Apr 5, 2017

Contributor

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

continue
if not len(dmap):
dmap[dmap._initial_key()]


def undisplayable_info(obj, html=False):
"Generate helpful message regarding an undisplayable object"

Copy path View file
@@ -20,8 +20,9 @@ class Stream(param.Parameterized):
the parameter dictionary when the trigger classmethod is called.
Depending on the plotting backend certain streams may interactively
subscribe to events and changes by the plotting backend. To disable
this behavior instantiate the Stream with linked=False.
subscribe to events and changes by the plotting backend. For this
purpose use the LinkedStream baseclass, which enables the linked
option by default.
"""

# Mapping from a source id to a list of streams
@@ -60,7 +61,7 @@ def trigger(cls, streams):
stream.deactivate()


def __init__(self, rename={}, source=None, subscribers=[], linked=True, **params):
def __init__(self, rename={}, source=None, subscribers=[], linked=False, **params):
"""
The rename argument allows multiple streams with similar event
state to be used by remapping parameter names.
@@ -149,7 +150,9 @@ def source(self):
@source.setter
def source(self, source):
if self._source:
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.

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

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

Copy link
@jlstevens

jlstevens Apr 5, 2017

Contributor

Ok, that makes sense.

self._source = source
self.registry[id(source)].append(self)

@@ -220,7 +223,18 @@ def __str__(self):
return repr(self)


class PositionX(Stream):
class LinkedStream(Stream):
"""
A LinkedStream indicates is automatically linked to plot interactions
on a backend via a Renderer. Not all backends may support dynamically
supplying stream data.
"""

def __init__(self, linked=True, **params):
super(LinkedStream, self).__init__(linked=linked, **params)


class PositionX(LinkedStream):
"""
A position along the x-axis in data coordinates.
@@ -232,7 +246,7 @@ class PositionX(Stream):
Position along the x-axis in data coordinates""", constant=True)


class PositionY(Stream):
class PositionY(LinkedStream):
"""
A position along the y-axis in data coordinates.
@@ -244,7 +258,7 @@ class PositionY(Stream):
Position along the y-axis in data coordinates""", constant=True)


class PositionXY(Stream):
class PositionXY(LinkedStream):
"""
A position along the x- and y-axes in data coordinates.
@@ -285,7 +299,7 @@ class MouseLeave(PositionXY):
"""


class PlotSize(Stream):
class PlotSize(LinkedStream):
"""
Returns the dimensions of a plot once it has been displayed.
"""
@@ -302,7 +316,7 @@ def transform(self):
'height': int(self.height * self.scale)}


class RangeXY(Stream):
class RangeXY(LinkedStream):
"""
Axis ranges along x- and y-axis in data coordinates.
"""
@@ -314,7 +328,7 @@ class RangeXY(Stream):
Range of the y-axis of a plot in data coordinates""")


class RangeX(Stream):
class RangeX(LinkedStream):
"""
Axis range along x-axis in data coordinates.
"""
@@ -323,7 +337,7 @@ class RangeX(Stream):
Range of the x-axis of a plot in data coordinates""")


class RangeY(Stream):
class RangeY(LinkedStream):
"""
Axis range along y-axis in data coordinates.
"""
@@ -332,7 +346,7 @@ class RangeY(Stream):
Range of the y-axis of a plot in data coordinates""")


class Bounds(Stream):
class Bounds(LinkedStream):
"""
A stream representing the bounds of a box selection as an
tuple of the left, bottom, right and top coordinates.
@@ -343,7 +357,7 @@ class Bounds(Stream):
Bounds defined as (left, bottom, top, right) tuple.""")


class Selection1D(Stream):
class Selection1D(LinkedStream):
"""
A stream representing a 1D selection of objects by their index.
"""
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.