Skip to content
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

Added Spikes Element #346

Merged
merged 16 commits into from Dec 11, 2015

Conversation

Projects
None yet
4 participants
@philippjfr
Copy link
Contributor

philippjfr commented Dec 10, 2015

This PR adds the new Spikes Element. The Spikes Element is a Columns type and draws vertical or horizontal lines at the specified locations, with fixed or variable height/width. I've also added some examples to the Element tutorial.

I've reproduced these examples here. Still have to update the Element reference_data and add some docstrings for the plot options but I'll wait on some feedback before doing that.

color_index = param.Integer(default=1, doc="""
Index of the dimension from which the color will the drawn""")

spike_height = param.Number(default=0.1)

This comment has been minimized.

@jlstevens

jlstevens Dec 10, 2015

Contributor

A better name for this would be nice although I don't have any better suggestions right now... spike_length maybe as that doesn't imply a direction?

This comment has been minimized.

@philippjfr

philippjfr Dec 10, 2015

Author Contributor

Certainly better. Will use that unless someone comes up with a better suggestion.

This comment has been minimized.

@jbednar

jbednar Dec 10, 2015

Contributor

Good point; sounds good.

@@ -157,7 +158,8 @@ def grid_selector(grid):


MPLPlot.sideplots.update({Histogram: SideHistogramPlot,
GridSpace: GridPlot})
GridSpace: GridPlot,
Spikes: MarginalRugPlot})

This comment has been minimized.

@jlstevens

jlstevens Dec 10, 2015

Contributor

Do we need the word Marginal? That depends on whether RugPlot can be used without being a side plot or not. If it is always a side-plot, RugPlot would do...

This comment has been minimized.

@philippjfr

philippjfr Dec 10, 2015

Author Contributor

Really it's just a subclass with different defaults, so it could be used separately but probably won't be in practice.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 10, 2015

Other than a few naming issues, this PR looks good. Ideally, we would find a better name than spike_height before merging...

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 10, 2015

I think it probably make sense to update the testing reference data first, make sure this PR passes (it should!) and then merge.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 10, 2015

Yes absolutely, just wanted to settle naming first.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 10, 2015

Btw, I wasn't able to allow setting the y-coordinates in screen coordinates because I found no easy way to do so for one axis but not the other in matplotlib. What I can offer is xunits and yunits plot options on Bokeh Elements which you can set to 'data' and 'screen', then there's a general mechanism for doing it in bokeh at least. In matplotlib I can add a different parameter that allows you to switch between data and axis/screen coordinates in general.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Dec 10, 2015

Looks great, and very useful!

I guess the implication of not being able to set the spike_length in screen coordinates is clear in the Bokeh rug plot example (second cell of your example notebook) -- zooming in should leave the rug where it is, i.e. it should be glued to the x axis, but instead zooming and panning move the rug around. Similarly, the marginal versions (last two cells) look fine at first, but in the Bokeh one it's clear that the rugs don't change to match the actual data being shown, so right now I wouldn't really consider it a rug plot, just something that looks like one under certain circumstances. A rug plot should just be an extra bit of info for whatever plot is being shown, always glued to the axis, always with the same height, and always matching the main plot.

Also, the marginal Bokeh plot (last cell in the notebook) is really ugly because of the useless axis lines and labels. I've noticed the same problem for Bokeh side histograms -- can't you suppress the extra labels, lines, and tickmarks from a side rug or a side histogram? They don't provide any useful information that I can see, since the main plot shows the axis ranges already (assuming you can fix the rugs always to match the main plot) and the heights are nominal (both here and for histograms). Plus the tick labels generally just overwrite each other, and and are thus unreadable even if they did have useful information. Is there a reason not to suppress all that info so that it looks more like the very-usable Matplotlib version?

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 10, 2015

I guess the implication of not being able to set the spike_length in screen coordinates is clear in the Bokeh rug plot example (second cell of your example notebook) -- zooming in should leave the rug where it is, i.e. it should be glued to the x axis, but instead zooming and panning move the rug around.

Right, in Bokeh I could theoretically enable it but then it'd be inconsistent between backends.

Similarly, the marginal versions (last two cells) look fine at first, but in the Bokeh one it's clear that the rugs don't change to match the actual data being shown, so right now I wouldn't really consider it a rug plot, just something that looks like one under certain circumstances.

They should be sharing axes and therefore zoom in and out together. Will have to find out where I broke that.

Also, the marginal Bokeh plot (last cell in the notebook) is really ugly because of the useless axis lines and labels. I've noticed the same problem for Bokeh side histograms -- can't you suppress the extra labels, lines, and tickmarks from a side rug or a side histogram?

Yes I can, in matplotlib I created a subclass with different defaults, will do something similar here.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 10, 2015

Right, in Bokeh I could theoretically enable it but then it'd be inconsistent between backends.

Inconsistency between backends is ok if one backend supports something the other one doesn't (i.e as is the case here). That said, we should try to enable features with additional parameters without modifying the semantics of existing parameters if possible.

For this reason, I would consider an extra parameter that enables this behavior (default False) to make this functionality available if Bokeh is being used while staying consistent with matplotlib by default. As always, the tricky thing is to find a decent parameter name!

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Dec 10, 2015

I think that in this case you do want behavior different in the different backends -- the matplotlib one looks fine as it is, because it's not possible to zoom in on it (or would it be possible, if the matplotlib figure were e.g. embedded in Qt instead of the notebook?). Without zooming, both plots are ok, but with Bokeh's zooming and panning it's clear that the rug doesn't work as a rug.

The subclass does sound useful for Bokeh; this will clean up various histograms that currently are confusing in the Bokeh version. Thanks!

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 10, 2015

Without zooming, both plots are ok, but with Bokeh's zooming and panning it's clear that the rug doesn't work as a rug.

As I said above that's something that should be fixed, was working at some point. Nevermind, get what you mean now. Yes to work as a rug the y-axis needs to be in screen coordinates.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 10, 2015

I'll repeat what I said above about the parameters:

What I can offer is xunits and yunits plot options on Bokeh Elements which you can set to 'data' and 'screen', then there's a general mechanism for doing it in bokeh at least. In matplotlib I can add a different parameter that allows you to switch between data and axis/screen coordinates for both axes.

Happy to rename these/change the API, but this is the level of control it should provide.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Dec 10, 2015

That sounds fine to me.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 10, 2015

What I can offer is xunits and yunits plot options on Bokeh Elements which you can set to 'data' and 'screen' ...

I think that is an interesting proposal and one worth considering. That said, I feel the issue of how to handle screen coordinates consistently (e.g for annotations and insets as well) is one that needs careful thought.

This would also be a general change that isn't specific to the Spikes element proposed in this PR. I would be happy to open a new issue to discuss this further (or a separate PR if you prefer) and I would also be happy to merge this PR once we've settled the naming issues (discussed in the comments above).

@philippjfr philippjfr force-pushed the spikes_element branch from 15efe63 to 1c047bb Dec 11, 2015

philippjfr added some commits Dec 11, 2015

class Spikes(Chart):
"""
Spikes is a 1D or 2D Element, which represents vertical or
horizontal ticks along some dimension. If an additional dimension

This comment has been minimized.

@jlstevens

jlstevens Dec 11, 2015

Contributor

To me 'ticks' makes me think about plotting... how about:

"Spikes is a 1D or 2D Element, which represents a series of vertical or horizontal lines distributed along some dimension..."

Philipp Rudiger added some commits Dec 11, 2015

Philipp Rudiger Philipp Rudiger
@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 11, 2015

PR is ready for merge.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 11, 2015

Looks good.

jlstevens added a commit that referenced this pull request Dec 11, 2015

@jlstevens jlstevens merged commit bafc20a into master Dec 11, 2015

3 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 increased (+0.1%) to 71.366%
Details

@philippjfr philippjfr added this to the v1.4.1 milestone Dec 11, 2015

@philippjfr philippjfr deleted the spikes_element branch Dec 11, 2015

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Dec 11, 2015

Can you update the demo notebook above to show how it works now?

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 11, 2015

Sure although apart from parameter naming not much has changed from those examples, since we weren't able to come to a conclusion on data vs. screen coordinates and apparently the MultiLine glyph doesn't support screen coordinates anyway. I can just rebuild the docs, then the examples will be available on holoviews.org/dev/Tutorials/Elements

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 11, 2015

Building the docs now and also updated the link above.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Dec 11, 2015

Hmm. Nothing seems to have changed, though, even things that don't seem to depend on the issue of data vs. screen coordinates -- there are still ticks, axes, and labels on the side rugs in the last plot, and those rugs are still not linked to the main plot so that they reflect the current pan or zoom. I would have thought the screen coordinate issue would only affect the second plot, where the rug is overlaid on the data plot?

In any case, would it work to have the two plots that are being overlaid here match only on one dimension, with an artificial extra dimension invented for the rug height so that it can be independent of the data coordinates? I.e., don't use genuine screen coordinates, but just something decoupled?

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 11, 2015

Hmm. Nothing seems to have changed, though, even things that don't seem to depend on the issue of data vs. screen coordinates -- there are still ticks, axes, and labels on the side rugs in the last plot, and those rugs are still not linked to the main plot so that they reflect the current pan or zoom. I would have thought the screen coordinate issue would only affect the second plot, where the rug is overlaid on the data plot?

Are you sure? I've fixed that and the page has updated for me. Can't remove the yaxes ticks though because then the plots grow to fill the gap. Also can't reduce the rugplot heights and any more because that results to unsatisfiable constraint errors in the bokehJS layout engine.

In any case, would it work to have the two plots that are being overlaid here match only on one dimension, with an artificial extra dimension invented for the rug height so that it can be independent of the data coordinates? I.e., don't use genuine screen coordinates, but just something decoupled?

They're currently only shared if their dimensions match, which won't be the case for the y-axis of the rug plots.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Dec 11, 2015

Strange; it never updates, no matter how many times I tell Chrome to refresh. I launched Firefox, and now it's updated, with the adjoint plots now correctly linked to the main plots. However, they still have distracting and superfluous visual elements:

blackfull

I was hoping for something more like:

black

(but with no extra axis lines to cause bogus ticks like in the above).

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 11, 2015

Hmm, may have to fiddle with the caching settings on my website.

I can at least get rid of the grid, and with some hacking I can make them appear a bit shorter. Nothing I can do about the remaining axes though, the plots will grow to take up the space of the x- and y-axis of the main plot if I disable them.

Edit: Actually thinking about it, I could fiddle the alpha of the ticks at least.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Dec 11, 2015

Can't remove the yaxes ticks though because then the plots grow to fill the gap.

I'm not sure what that means, but any extra marks are confusable with actual data items, which makes this not be very usable in the Bokeh version. Maybe we have to give up on saying that Bokeh supports rugs.

Also can't reduce the rugplot heights and any more because that results to unsatisfiable constraint errors in the bokehJS layout engine.

Sounds like something to report; again with this height they are not very usable; they are meant to be a handy way to appreciate the marginal distribution, not something that balloons the overall plot size.

They're currently only shared if their dimensions match, which won't be the case for the y-axis of the rug plots.

Here I was talking about the second plot on the page, with the rug overlaid. I thought that even sharing one axis was enough to link plots, so far as I've seen in examples, but just for that one axis.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Dec 11, 2015

Sounds to me like we should say that only Matplotlib supports rug plots, and that they will need extra work in the Bokeh version (which may depend on the PhosphorJS implementation).

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 11, 2015

I'm not sure what that means, but any extra marks are confusable with actual data items, which makes this not be very usable in the Bokeh version. Maybe we have to give up on saying that Bokeh supports rugs.

As suggested above I could fiddle the alpha of the ticks and at least make them appear shorter.

Sounds like something to report; again with this height they are not very usable; they are meant to be a handy way to appreciate the marginal distribution, not something that balloons the overall plot size.

True, will ask on the bokeh flowdock first.

I thought that even sharing one axis was enough to link plots, so far as I've seen in examples, but just for that one axis.

That's correct.

Sounds to me like we should say that only Matplotlib supports rug plots, and that they will need extra work in the Bokeh version (which may depend on the PhosphorJS implementation).

If I can't make the alpha fiddling and height adjustment work I'll do that instead.

@bryevdv

This comment has been minimized.

Copy link

bryevdv commented Dec 11, 2015

Quick suggestion: use Segment glyphs to draw the rug spikes on one plot (not on plots to the side). If putting them over the central plot with some alpha or whatever will work, that is absolutely the simplest case. If need be, I think we can adjust the plotting layer so that the default central plot area clipping is disabled, then they could appear "below" or "to the "side" outside of the central plot area. This would be easiest on the sides where there is not an axis, but we could probably also tinker with axis tick and standoff values if necessary. First step: just get it working with segments inside the main plot.

@bryevdv

This comment has been minimized.

Copy link

bryevdv commented Dec 11, 2015

FYI slightly longer term plan for Bokeh: make a "rug" annotation, and then use that to implement ticks on axes (so that axes becomes simpler in the process, it's currently one of the most complicated/heavyweight models in bokehjs)

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 11, 2015

@bryevdv Thanks for the quick reply, using Segment makes total sense and should be pretty straightforward. Is there already an easy way to toggle the plot area clipping as that might be useful for me in general? Creating a rug annotation glyph in bokeh will also be really useful, but for now this approach sounds good.

@bryevdv

This comment has been minimized.

Copy link

bryevdv commented Dec 11, 2015

@philippjfr whether clipping is on or not is controlled by the plotting "level". You can see here:

https://github.com/bokeh/bokeh/blob/master/bokehjs/src/coffee/common/plot.coffee#L376

That "overlay" and 'tool" layers are the ones that are not clipped during rendering. I'd suggest something like:

p = figure()
r = p.segment(...)
r.level = "overlay"

Fair warning: this plumbing has not been exercised much, we may uncover small bugs, but I think it should be OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.