Adding datashade and rasterize options to scatter_matrix

[ppwadhwa]

Fixes #114

@tonyfast, @jlstevens and I worked through a solution to add a datashade option for a hvplot scatter_matrix. @jlstevens pointed out datashade options (aggregator, rasterize, etc) may need to be considered when implementing this. This PR, so far, only includes an argument to set datashade = True in the call to scatter_matrix, which will then use grid.apply(datashade, per_element=True), as @philippjfr suggested as a solution in the issue.

[jlstevens]

@philippjfr Looks like the travis builds are failing due to some python version issue. @ppwadhwa This means you will need to rebase this PR once this issue is fixed on master.

[jlstevens]

I've just realized that although the name is scatter_matrix and it does have scatters, there is also the diagonal to think about:

Therefore, unless there are appropriate elements along the diagonal that can support datashader (I don't think histograms do, KDEs maybe?), only the non-diagonal scatters are likely to be datashaded right now, leaving the diagonals to be rendered with bokeh. Do you agree @philippjfr ?

[philippjfr]

I do agree, yes.

[jbednar]

The diagonals don't need Datashader, as they are already aggregated and thus independent of the dataset size. The scatterplots do need Datashader, as they are dependent on the dataset size.

[ppwadhwa]

I've made a few updates, so that I could include a few keyword arguments for datashade, dynspread, and rasterize. I only have a couple of options, however, I wanted to get some feedback on whether what I have is going in the right direction, or if there is a totally different way to approach this problem.

[coveralls]

Coverage decreased (-1.5%) to 70.953% when pulling 4577a2a on ppwadhwa:iss114 into 17ce0cc on holoviz:master.

[jlstevens]

I only have a couple of options, however, I wanted to get some feedback on whether what I have is going in the right direction, or if there is a totally different way to approach this problem.

@philippjfr may have a different opinion but what I see in the diff looks reasonable to me!

[ppwadhwa]

@jlstevens @jbednar thanks for looking this over! @jbednar I made some updates per your comments. Please take a look and tell me what you think. thank you!

[jbednar]

Looks good!

[ppwadhwa]

Note on a need to do: I don't have dynspread or spread working for the rasterized plot. @jlstevens just told me that an updated version of datashader should support this, so I will update datashader, and add these options back in.

[jbednar]

Looks good to me, thanks so much! I've suggested a way to reduce having so many code paths at the end by using function pointers (including a no-op for spreading when there is no spreading); hopefully you can see how it works. If it still works after applying these changes, I'm happy to merge it!

[jbednar]

I've reviewed this and tried it out. Seems to work great! The only problem I can see is that the plot isn't dynamic. Normally in hvplot, datashaded and rasterized plots set dynamic=True so that they will update automatically on zooming by default. Here, it doesn't seem to re-render on zooming, and also doesn't accept the dynamic parameter so we can't enable being dynamic.

@philippjfr , what needs to be done to support dynamic?

[ppwadhwa]

Thanks for doing all this, @jbednar ! It looks much prettier with your changes. :)

[philippjfr]

So I finally tracked down why this was such a nightmare. Specifically on the HoloViews side any layout or grid of plots tries to merge stream callbacks that have the same Bokeh models as their source. The problem was that it even merged stream callbacks that only partially overlapped in their handles, e.g. if you had two RangeXY callbacks attached to separate plots, if both were attached to the same Range1D object they would get merged. This is obviously incorrect since a) the fact that the x_range is the same does not mean the y_range will also match and b) even if x_range and y_range match they might be transposed.

I've pushed holoviz/holoviews#5133 which will have to be backported to HoloViews 1.14.6

[philippjfr]

@maximlt will rebase the PR and then let's merge it. Fixing the bug in HoloViews doesn't have to hold up this PR.

[jlstevens]

Will you want to pin HoloViews to 1.14.6 before the next hvplot release or maybe just warn when using hvplot.scatter_matrix when older HoloViews versions are in use?

[maximlt]

Now the CI is green (well there's some coverage decrease, despite the fact that I've added quite a number of tests 🤔 ) does any of one of you @philippjfr or @jbednar want to review it?

The biggest change I've made was to wrap the rasterize/datashade and dynspread/spread operations into a partial to allow to pass keyword arguments to the operations, since grid doesn't accept keyword arguments.

[jbednar]

Looks good! I made various trivial comments and improvements that should be able to be applied easily, and then I'm happy for this to be merged.

[philippjfr]

Going to merge! Sorry it took so long @ppwadhwa, turns out the issue was in HoloViews. Thank you!

[maximlt]

Yes thanks to everybody who contributed to this PR!!!