Show all histograms of an Overlay

[peterroelants]

I created a PR to enable showing all histograms of an Overlay:

This PR:

• Fixes NdOverlay.hist raises AttributeError: NdOverlay.hist raises AttributeError #5022 . NdOverlay.hist works in this PR.
• New behaviour: CompositeOverlay.hist() shows histograms for all the elements in the overlay. This affects both NdOverlay and Overlay (e.g. see previous comment @ NdOverlay.hist raises AttributeError #5022 (comment))

Illustration of new CompositeOverlay.hist() behavior. Notice the histograms being overlaid for both elements in the overlay:

I would love to get some feedback on this PR

[peterroelants]

I think I still have to write/update some tests related to this. However, I couldn't find a contributor's guide on the Holoviews website that explains the Holoviews testing frameworks and principles. Can anyone provide me a reference to how to write and run tests for Holoviews?

[peterroelants]

Comparison of new vs old behavior for the regular Overlay from the snippet:

import numpy as np
import holoviews as hv

hv.extension('bokeh')


pts1 = hv.Points(np.random.randn(1000, 2) , kdims=['x', 'y'], label='p1l')
pts2 = hv.Points(np.random.randn(1000, 2) - 5, kdims=['x', 'y'], label='p2l')
pts_overlay = pts1 * pts2
plot = pts_overlay.hist(dimension=['x','y'])
print(repr(plot))
plot

• Old behavior:
  
• New behavior (this PR):
  

I also have a notebook that illustrates in more depth some of the features of this PR: https://gist.github.com/peterroelants/32fadcdf4d27ff6228178fa9a8a2252b#file-overlay_hists-ipynb

You have to run the notebook yourself on this branch to see the effects (GitHub Gists crashed each time when I tried to upload the notebook with embedded Holoviews/Bokeh plots)

EDIT: Update figure after updating PR

[peterroelants]

Some things to be considered:

• Is it possible to inherit colormaps from the parent plot in some way? If so, how might I enable this?
• Currently, the legend of the overlay is replicated on each plot of the Adjoint (when adjoin=True), do we want this as a default? Or do we only want to show the legend on the main plot?

[jbednar]

I'm traveling and only have my phone, but from the pictures it looks like a big improvement to me! Maybe @jlstevens or @philippjfr will be able to take a look sooner.

[jbednar]

One comment: it might be good to suppress the legend for the histogram, because it's redundant for an AdjointLayout and typically overlaps the bars a lot. ETA: now your own comment shows up on my phone saying the same thing about the legends; yes, I do think they should be suppressed by default. I don't have an answer for the colormap question. Sorry for the confusing reply!

[peterroelants]

@jbednar I updated this to hide the legend in the histograms by default, and added a parameter to still enable them if needed. I also made sure that axes are aligned in the case of adjoin=True

One weird thing that I cant explain is that the histogram plot sizes of Overlays are different compared to the histograms of plain elements. E.g.:

vs

Please take your time while travelling. I'm not in a rush to get this in.

[philippjfr]

I think this is a sensible change but since it's a change in behavior would go into 2.0.

[peterroelants]

I think this is a sensible change but since it's a change in behavior would go into 2.0.

@philippjfr Should I file a PR against a different branch? Or leave this as-it-is for now?

[philippjfr]

You're good, master is already in "2.0" mode, e.g. a bunch of deprecations for 2.0 have already happened.

[jlstevens]

Thanks for the PR!

This change seems appropriate to me and as already mentioned this is a good time to allow backwards incompatible behavior.

I've not yet checked what unit tests are running here but I would want some tests that make sure index still works to select a particular layer when necessary. Otherwise I am happy with the changes made in this PR.

[jbednar]

Looks great to me! @jlstevens , can you see how to avoid having the size change like that? The smaller size (equivalent to adding .opts(width=120) (or so) if I recall correctly) seems much more appropriate for an adjoint histogram, but I don't know why the size is changing when it's an overlay.

[peterroelants]

@jlstevens I rebased this PR on latest master, and most tests seem to be passing. The ones that fail seem to fail due to unrelated reasons:

• Out of memory in
• Plotly import error
• Killed during conda install

All of them are Python 3.6 tests

[peterroelants]

I would love to get this merged in at some point.

Is there anything that I can do on my side to get this approved and merged?

[jlstevens]

Sorry for letting this languish!

I agree that those test failures are unrelated. I will now approve this PR (looks fine to me) but I will wait a bit to give @philippjfr to also have a look. I'll merge at the end of the day regardless.

[jlstevens]

Looks good to me!

I don't think we need to worry about backwards compatibility as it will end up on the 2.0 branch. The only thing that would be nice to add would be some additional unit tests...

[jlstevens]

Actually, there is the size issue that @jbednar mentions. I'll have a look now...

[jlstevens]

The sizing issue is unrelated to this PR: the problem is about how overlays become adjoint which is a separate problem.

@peterroelants Can you have a look at the issues and report this if it hasn't been already? At which point I am happy to merge.

[peterroelants]

@jlstevens I couldn't find an existing issue related to the adjoint overlays resulting in an unexpected plot size. I created a issue for it at #5072

[jlstevens]

Thanks!

I'm going to go ahead and merge. That way we can test and iron out any other issues before the 2.0 release.