Avoid rerendering of overlaid DynamicMaps with non-triggered streams

[philippjfr]

This PR addresses a very hairy issue that's probably responsible for a fair number of bugs. I'll first outline the issue and then describe the solution I've come up with. This PR will require some very thorough testing since it messes with some very central code.

The main issue is that when a DynamicMap returns an Overlay it is not immediately clear whether the constituent parts of that Overlay were each supplied by another DynamicMap or whether the user defined callback directly returns an Overlay. Secondly it is worth knowing that when a DynamicMap that returns Overlays has streams on it, the streams will trigger updates on the whole OverlayPlot rather than the specific ElementPlot it might have been originally attached to (for important reasons I won't get into). This means that all the items in the Overlay will get re-rendered even if they shouldn't be. This rerendering on it's own can have implications when there are complex stream dependencies between different elements, e.g. when two layers are linked bi-directionally. In the worst case this will result in loops, while in the best case it will break the rendering of the plot.

The secondary issue is that each subplot of the OverlayPlot is given a DynamicMap that does not necessarily correspond to the thing it's actually rendering. This in turn can cause validation issues if that DynamicMap is actually called (which it ideally shouldn't be) because the thing it's returning may not match what has been put in its cache. This is because DynamicMap does not have its own split_overlays method implementation and simply uses the HoloMap.split_overlays method which is technically incorrect (but does sort of work in most scenarios).

The aim then should be for the OverlayPlot to only trigger rendering of the specific Elements that should actually be redrawn. However due to the way the OverlayPlot tries to split up the overlaid DynamicMap it actually has no idea which component triggered the update. The appropriate solution therefore is for the split_overlays method on DynamicMap to correctly split up the DynamicMap into the original DynamicMaps that the user composed using the * operator.

This should result in more optimized updates and fix a number of annoying bugs.

Related to #493 and #2174

[jlstevens]

The appropriate solution therefore is for the split_overlays method on DynamicMap to correctly split up the DynamicMap into the original DynamicMaps that the user composed using the * operator.

This does sound like the correct semantics that I would expect to be obeyed anyway. It is good that this issue has been identified but it is a hairy issue as you say so all I can suggest is add as many unit tests as possible, then we should merge ASAP and continue testing.

[philippjfr]

Okay backtracking here, while the idea about making DynamicMap.split_overlays work correctly is a nice one, I've now decoupled that work from this PR. It's particularly hairy stuff and should not be bound up with the actual problem I was trying to address here, which is to ensure that each plot only references those streams that are actually driving its updates and skipping updates to subplots, which have not been updated by a change in an existing stream.

It's taken me a long time to work this stuff out, but at least the new plotting utility is very, very well tested now.

[philippjfr]

Ready for review and merge.

[jlstevens]

I would consider creating an issue discussing the limitations/problems with this utility and referencing it in the docstring. It might be a useful reminder for the next person looking at this code.

[philippjfr]

I'm not aware of any limitations, but I'm happy to expand the docstring.

[jlstevens]

Okay backtracking here, while the idea about making DynamicMap.split_overlays work correctly is a nice one, I've now decoupled that work from this PR.

Ok. In that case please make sure there is an issue describing the problem. I might even reference this issue in the utility docstring to inform the next person who looks at that bit of code (see comment above).

It's taken me a long time to work this stuff out, but at least the new plotting utility is very, very well tested now.

Good to see all those tests, but it tends to be the edge cases you haven't thought to test that catch you out later! ;-p

Other than the docstring update suggestion, the code looks ok to me so I am happy to merge. Best to get this tested in practice ASAP.

[philippjfr]

I might even reference this issue in the utility docstring to inform the next person who looks at that bit of code (see comment above).

The two issues are now completely unrelated, as they should be. Trying to consider and fix both problems at the same time caused lots of headaches.

To be more precise, it is not possible for the DynamicMap passed to each subplot to both return the elements that are actually being plotted by that subplot and for it to reference the streams that drive it. By separating the stream assignment logic, it should now be much easier to achieve the former and have DynamicMap.split_overlays work more correctly.

[jlstevens]

Oops. I meant "I might even reference the issue in the utility docstring" where there is an appropriate (new) issue.

Anyway, it was just a suggestion so I don't mind if you ignore it.

[philippjfr]

I meant "I might even reference the issue in the utility docstring" where there is an appropriate (new) issue.

I got that, my point was that the new issue I'll be creating will have nothing to do with the utility, so there's no reason to reference it.

[philippjfr]

Ready to merge now.

[jlstevens]

Ok. Best to merge now to maximize the real world testing we can do before the next release.

[ayoungs]

InteractiveBoxTestApp.py.zip
This isn't working, a simple bounds overlay update with box tool selection, it won't work in a stand a alone app, but this type of thing works fine in Jupiter notebooks.

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