Support link_selections on dynamic overlays

[jonmmease]

This PR adds support for performing link_selections on DynamicMaps that are the result of performing the __mul__ operator on one or more DynamicMap layers.

This came up when trying to use link_selections and datashader together on top of a Tiles element.

e.g.

link_selections(tiles * datashade(...))

The result of tiles * datashade(...) is a DynamicMap that returns an Overlay. Previously we skipped these in link_selections because it's not possible to accurately recurse in the general case.

This PR handles only the very common case of DynamicMaps generated by the __mul__ operator.

[jbednar]

This PR handles only the very common case of DynamicMaps generated by the __mul__ operator.

Thanks! What cases wouldn't be handled? I had thought that an hv.Overlay generated with the __mul__ operator was the same as one generated any other way.

[jonmmease]

I had thought that an hv.Overlay generated with the mult operator was the same as one generated any other way.

That's what I had thought as well, but turns out that hv.Overlay([dmap, el]) returns on hv.Overlay consisting of an Element and a DynamicMap while dmap * el returns a DynamicMap that produces an overlay.

Thanks! What cases wouldn't be handled?

It doesn't handle the general case where someone writes a custom dynamic map callback function that returns an overlay, but I'm not sure there is a meaningful way to handle this case.

[jbednar]

I wonder if the regular Overlay constructor should be extended to do the same as __mul__ when it encounters a DynamicMap. @jlstevens ?

[jlstevens]

That's what I had thought as well, but turns out that hv.Overlay([dmap, el]) returns on hv.Overlay consisting of an Element and a DynamicMap while dmap * el returns a DynamicMap that produces an overlay.

That is correct. The issue here is that __mul__ operator has the freedom to switch the resulting type while the Overlay constructor will always return an Overlay (without some heavy magic). @philippjfr and I discussed trying to pull out the machinery of __mul__ into an operation but this is quite tricky unfortunately.

Nothing stops us having an overlay utility that simply applies __mul__ to the supplied elements/dynamicmaps but I'm not sure whether that will help here (it would probably also be confusing). Of course Overlay could also have a classmethod that does this (probably a better idea) but again, I'm not sure how much this would help here.

The only other thought I have is that you could detect when you are supplied dmap * el (or similar) and rejig that to be a single dynamicmap internally. That would only work if an internal clone will do the job of course...

[philippjfr]

We could easily add a __new__ implementation that does the right thing for hv.Overlay([el, dmap]), which is probably preferable over asking the user to call collate in that case.

[jlstevens]

I'm not sure how I feel about constructors that give you a different type as a result. Also, dmap * el is a perfectly valid structure that also has its uses. How would you toggle between these behaviors?

[philippjfr]

Not sure what you mean by the second point. What I was suggesting wouldn't affect the * behavior so what would be toggled between?

[jlstevens]

I think there are cases where hv.Overlay([el, dmap]) defining an overlay of an element and a DynamicMap is a perfectly reasonable thing to want (instead of returning a DynamicMap).

I suppose this structure tells you to run .collate? In which case it might be more user friendly to just return a DynamicMap but I still don't like it when the constructor gives you something that doesn't work with isinstance. Prompting users to call .collate explicitly gives users the flexibility of using the overlay as an intermediate datastructure though maybe that is less useful...

[philippjfr]

I suppose link_selections can just do the introspection and apply the collate itself so a user doesn't have to.

[jlstevens]

Right.

I suppose the question is now whether:

1. Users should call .collate before using link_selections (doesn't affect this PR, doesn't affect HoloViews other than maybe docs)
2. Whether link_selections can just apply .collate (affects this PR, doesn't affect the rest of HoloViews)
3. Whether we should change Overlay (doesn't affect this PR, affects the rest of HoloViews)

I vote for option 2 as I think it shouldn't be too hard to do and I think option 3 is a big drastic right now.

[jbednar]

Thanks for explaining all that; I did suspect it was related to collate(). What I missed was that * wasn't returning an Overlay, even though Jon did say that. :-) I vote for option 2; if we detect in link_selections that it needs collating first, just collate it.

If link_selections does that, are there then any remaining overlay-like things (i.e. Overlays and things dynamically returning Overlays) that won't be supported for link_selections, assuming the things in the overlay are individually supported?

Also, what about elements in the Overlay that aren't supported for link_selections? I would hope that those can just be ignored, so that their presence does not break the overall usability of the overlay. Maybe it should warn for such Elements, so that the user knows some elements are being ignored, but allow the warning to be silenced with an explicit argument to link_selections. (And the warning could mention the name of that argument, so that it's easy for the user to do the right thing.) The reason I'm asking is that previously using link_selections with an overlay was very awkward and required learning new concepts (grabbing the ls object and applying it individually to the bits that work), and I'd really hope that only really weird circumstances would need special treatment like that.

[jonmmease]

2. Whether link_selections can just apply .collate (affects this PR, doesn't affect the rest of HoloViews)

The trouble is that, currently, collate doesn't do anything with DynamicMaps that return overlays:

import holoviews as hv
from holoviews.operation.datashader import datashade
from holoviews.plotting.util import initialize_dynamic

dmap_overlay = hv.Points([(0, 0)]) * datashade(hv.Points([0, 0]))
dmap_overlay

:DynamicMap   []
   :Overlay
      .Points.I :Points   [x,y]
      .RGB.I    :RGB   [x,y]   (R,G,B,A)

dmap_overlay.collate()

:DynamicMap   []
   :Overlay
      .Points.I :Points   [x,y]
      .RGB.I    :RGB   [x,y]   (R,G,B,A)

We could potentially move the approach that I used in this PR to update collate to return an Overlay of Points and DynamicMap in this case, but that seems like a larger change than just handling it in link_selections for now. Perhaps we could record this as something to revisit in version 2.

If link_selections does that, are there then any remaining overlay-like things (i.e. Overlays and things dynamically returning Overlays) that won't be supported for link_selections, assuming the things in the overlay are individually supported?

I think this covers everything that we can reasonably cover. Elements in the overlay (dynamic or not) that don't support selections are retained unmodified. There is not warning at the moment though. This worked previously with the result of Overlay([...]), but in the case of the __mul__ operator with one or more DynamicMap objects, the whole overlay was ignored. With this PR both cases should work the same way.

As I mentioned before, if someone builds a DynamicMap based on a custom callback function that happens to return an Overlay, there isn't really anything we can do to introspect into the function. So DynamicMaps built this way will still be returned unmodified.

[jlstevens]

I must be confused or missing something...

I thought the case that needs collate is hv.Overlay([el, dmap]) and that with this PR everything worked when using __mul__? When using __mul__ I wouldn't expect collate to change anything which is what you show in your example...

[philippjfr]

Right, that was my understanding as well, specifically you said:

This PR handles only the very common case of DynamicMaps generated by the __mul__ operator.

So why would collate be needed for that case at all? Our suggestion was to handle hv.Overlay([el, dmap]) using the same approach as you are implementing here, by calling collate first, which should make it behave just like an Overlay created using __mul__.

[philippjfr]

I guess you assumed that we were suggesting that collate could somehow replace what you've done here, which is not the case. We were simply suggesting that we could cover the hv.Overlay([..., dmap, ...]) as well by combining collate with what you've done here.

[jonmmease]

Ohh, I see. Thanks for clarifying. I was confused because the link_selections(hv.Overlay([dmap, el])) case already worked (before this PR) without calling .collate(). It's handled here:

holoviews/holoviews/selection.py

Lines 187 to 189 in 886b03d

	elif isinstance(hvobj, (Layout, Overlay, NdOverlay, GridSpace)):
	data = OrderedDict([(k, self._selection_transform(v, operations))
	for k, v in hvobj.items()])

This makes sense to me because it's the same logic path that regular (non-dynamic) overlays follow.

But yeah, if you all want I think we could probably call .collate on everything at the start of link_selections to steer that case through the code I added in this PR. But this might have other consequences for other input types that I don't foresee right now. It would be a larger change with more risk of regression in my opinion, but does that sound preferable?

[jbednar]

Right -- that's what I'm after. Keeping all the progress you've made in this PR, but using collate on Overlays so that it can go even further and cover more cases (hence my question about what's still not covered if this extension is made to the PR).

ETA: this comment was written before Jon's; it is replying to Philipp, not Jon!

[jbednar]

It would be a larger change with more risk of regression in my opinion, but does that sound preferable?

I think it's worth a try, because it addresses a class of inputs that we know is going to fail, so it's a known fix against a possible but unknown possibility of breakage. We can always revert it, but it does seem worth trying!

[jonmmease]

I think it's worth a try, because it addresses a class of inputs that we know is going to fail

It's not clear to me what these cases are. The current state of this PR handles __mul__ and Overlay([...]) for dynamic and non-dynamic inputs.

[jbednar]

What cases wouldn't be handled?

It doesn't handle the general case where someone writes a custom dynamic map callback function that returns an overlay, but I'm not sure there is a meaningful way to handle this case.

I think the proposal is to call .collate() for this case and thereby cover it. Right?

[jonmmease]

I'll think on it some more, but right now I don't see how collate would help with that case. @philippjfr and @jlstevens?

[philippjfr]

Yeah, I hadn't realized hv.Overlay([..., dmap, ...]) was already covered but that does make sense. I'm -1 on adding collate too then since it could cause some regression. Sorry for even bringing it up since it derailed discussion a fair bit.

[jonmmease]

Sorry for even bringing it up since it derailed discussion a fair bit.

No, I appreciate you weighing in. There's a lot going on here so I really appreciate whatever comes to your mind 🙂

[philippjfr]

The MacOS failures are unrelated. Merging.

[jonmmease]

Thanks, @philippjfr!

[github-actions]

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.