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

Avoid rerendering of overlaid DynamicMaps with non-triggered streams #2253

Merged
merged 1 commit into from Jan 11, 2018

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Jan 10, 2018

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

@philippjfr philippjfr added the bug label Jan 10, 2018

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 10, 2018

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

This comment has been minimized.

Member

philippjfr commented Jan 11, 2018

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

This comment has been minimized.

Member

philippjfr commented Jan 11, 2018

Ready for review and merge.

@philippjfr philippjfr added this to the v1.10 milestone Jan 11, 2018

"""
Splits a DynamicMap into the original component layers it was
constructed from.
"""

This comment has been minimized.

@jlstevens

jlstevens Jan 11, 2018

Member

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.

This comment has been minimized.

@philippjfr

philippjfr Jan 11, 2018

Member

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

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 11, 2018

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

This comment has been minimized.

Member

philippjfr commented Jan 11, 2018

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

This comment has been minimized.

Member

jlstevens commented Jan 11, 2018

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

This comment has been minimized.

Member

philippjfr commented Jan 11, 2018

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

This comment has been minimized.

Member

philippjfr commented Jan 11, 2018

Ready to merge now.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 11, 2018

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

@jlstevens jlstevens merged commit 0813737 into master Jan 11, 2018

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 remained the same at 81.255%
Details
s3-reference-data-cache Test data is cached.
Details
@ayoungs

This comment has been minimized.

ayoungs commented Jan 12, 2018

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.

@philippjfr philippjfr deleted the dynamicmap_overlay_stream_updates branch Jan 13, 2018

@philippjfr philippjfr modified the milestones: v1.10, 1.9.3 Feb 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment