Allow dynamically adding new subplots

[philippjfr]

This PR allows OverlayPlots to dynamically add new subplots when a DynamicMap returns more elements than were present on initialization. So far this is very much WIP and requires thorough testing. It does seem to work based on initial testing and should be a huge generalization of DynamicMaps.

Addresses: #2097, #1388, #493

[philippjfr]

Here is a simple test case:

def cb(X):
    return hv.Overlay([hv.Curve(np.arange(10)+i) for i in range(X)])
hv.DynamicMap(cb, kdims=['X']).redim.range(X=(1, 10))

[jbednar]

Fabulous, thanks!

[jlstevens]

Very happy to see we will be able to support this finally.

I agree this needs thorough review though maybe it might be better to merge to master (after 1.9.2) ASAP to make sure it is thoroughly tested before the next release? I suspect there are only so many issues we will be able to uncover if we leave this feature in this PR too long...

[philippjfr]

Agreed, I'll write some unit tests as soon as I have a minute and then we can hopefully merge this. I don't think it should change anything for things that were already supported but I expect there may be some things that could be improved when adding new subplots.

[jlstevens]

It might also be worth adding a few examples to the docs to demonstrate that the old constraint is now removed. I'm not quite sure where though...

[basnijholt]

This is (almost) exactly what I need 👍

In adaptive we have a live plotting functionality and depending on the data that is returned we need to decide how to plot it. Sometimes we for example need a Path and other times a Scatter, however before any data is known a user might have already called the live_plot, so we might need to change the plot type.

The logic is in the following code:

import holoviews as hv
hv.notebook_extension('bokeh')

def plot():
    while True:
        if path:
            yield hv.Path([])
        else:
            yield hv.Scatter([])

path = True
dm = hv.DynamicMap(plot(), streams=[hv.streams.Stream.define('Next')()])
dm

dm.event()  # works
path = False
dm.event()  # should plot a Scatter, but errors

With this PR a DynamicMap only allows Overlays to be changed, so my example would work if we do:

        if path:
            yield hv.Overlay([hv.Path([])])
        else:
            yield hv.Overlay([hv.Scatter([])])

which seems a bit unnessecary.

[basnijholt]

Just to strengthen my point, I would like to describe how we solve the above issue at the moment.

We do it by creating an overlay:

    def plot(self):
        if not self.data:
            return hv.Scatter([]) * hv.Path([])
        elif self.vdim > 1:
            return hv.Scatter(self.data) * hv.Path([])
        else:
            xs = list(self.data.keys())
            ys = list(self.data.values())
            return hv.Path((xs, ys)) * hv.Scatter([])

This leads to all kinds of annoying problems, like having to access attributes as learner.plot().Scatter.I.opts(style=...) instead of just learner.plot().opts(style=...).

Or for example this issue #2347.

Allowing plot types to be changed in a DynamicMap would make life easier :)

[philippjfr]

Due to a regression in bokeh this is once again blocked: bokeh/bokeh#7590

[philippjfr]

I'm sort of tempted to go ahead with this despite the bokeh regression. If a user doesn't make use of the functionality there is no change and if they do they lose the toolbar but otherwise should see no issue. Since this is a feature this would otherwise have to wait on an 1.11 release, while bokeh could fix the underlying issue quite soon.

[basnijholt]

Then I would say to just include it! 😁

[philippjfr]

Thoughts @jbednar and @jlstevens?

[jlstevens]

I suppose if the release notes/changelog explicitly mention the problem with the toolbar disappearing, I would also be inclined to include it sooner rather than later.

[basnijholt]

I see you amended there commit recently, did you by any chance address my comment #2174 (comment) (sorry, I'm AFK so can't test it.)

[philippjfr]

@basnijholt For some reason I can't tell exactly which comment you're referring to but what you describe in the comments above is exactly what this PR should address.

[philippjfr]

Okay the main reason I'm +1 on merging this is that it doesn't really affect cases that were previously already supported at all, so will not cause any regressions for existing use cases. The release notes should clearly point out the limitations with bokeh but otherwise I think this is in good shape.

In addition to the new functionality I have also improved existing functionality. OverlayPlots already supported remapping existing subplots if they were not being used, while this worked it also had some bugs, e.g. legend and the cyclic_index were not updated appropriately resulting in the wrong color being used and the legend not updating. I've now addressed this and added unit tests for this case as well.

[philippjfr]

Ready to review.

[basnijholt]

@basnijholt For some reason I can't tell exactly which comment you're referring to but what you describe in the comments above is exactly what this PR should address.

@philippjfr I just tried to run the code snippet I posted in #2174 (comment) and it seems that it doesn't work yet.

See the traceback:

Traceback (most recent call last):
  File "/Users/basnijholt/Sync/Work/holoviews/holoviews/plotting/util.py", line 258, in get_plot_frame
    return map_obj[key]
  File "/Users/basnijholt/Sync/Work/holoviews/holoviews/core/spaces.py", line 1122, in __getitem__
    self._cache(tuple_key, val)
  File "/Users/basnijholt/Sync/Work/holoviews/holoviews/core/spaces.py", line 1169, in _cache
    self[key] = val
  File "/Users/basnijholt/Sync/Work/holoviews/holoviews/core/ndmapping.py", line 520, in __setitem__
    self._add_item(key, value, update=False)
  File "/Users/basnijholt/Sync/Work/holoviews/holoviews/core/ndmapping.py", line 157, in _add_item
    self._item_check(dim_vals, data)
  File "/Users/basnijholt/Sync/Work/holoviews/holoviews/core/ndmapping.py", line 821, in _item_check
    (self.__class__.__name__, type(data).__name__, self.type.__name__))
AssertionError: DynamicMap must only contain one type of object, not both Scatter and Path.

[philippjfr]

I see, I believe this will separately be fixed by #2286, will try that in a minute.

[basnijholt]

I just tried rebasing on the branch of #2286 but I get the same error.

[philippjfr]

Damn, thanks for checking I'll have a look.

[philippjfr]

Sorry I should have read your question more clearly, wrapping the elements in an Overlay to be able to change the types will indeed continue to be required. I'd be happy to potentially address this limitation by always automatically wrapping individual elements in an Overlay, but that's definitely out of scope for this PR as it's a far more wide-reaching change with the potential for a lot of unintended side-effects.

[jlstevens]

Looks ok. Ready to merge when the tests pass?

[philippjfr]

Yes, let's go ahead. It's a relatively safe change on its own, we just need to remember to mention the caveats in the release announcement and I'll open an issue to continue discussion on the issue @basnijholt mentioned.

[philippjfr]

Have to rebase and start tests over unfortunately, the colormap test changes causing issues.

[philippjfr]

I've opened the issue now, and tests appear to be passing.

[jlstevens]

Merging.

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