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

Regression: spikelines no longer supported on bar charts #2336

Closed
shogun70 opened this issue Feb 6, 2018 · 6 comments
Closed

Regression: spikelines no longer supported on bar charts #2336

shogun70 opened this issue Feb 6, 2018 · 6 comments
Labels
bug something broken regression this used to work

Comments

@shogun70
Copy link

shogun70 commented Feb 6, 2018

I've been trying out the latest release (v1.33.1) and found that spikelines no longer appear on bar-charts.

This seems to be a regression from the current version I'm using (v1.30.1)

I've made what is hopefully an illustrative demo:
https://codepen.io/anon/pen/gvMBYR

@alexcjohnson
Copy link
Collaborator

Thanks for the report. bar with spikelines works through v1.32 but breaks in v1.33

@alexcjohnson alexcjohnson added bug something broken regression this used to work labels Feb 6, 2018
@etpinard
Copy link
Contributor

etpinard commented Feb 6, 2018

Thanks for the report @shogun70

I'll try to get a fix in before releasing 1.34.0.

@etpinard
Copy link
Contributor

etpinard commented Feb 6, 2018

Ok. So the culprit (no surprises) is PR #2247

In fact, all non-scatter cartesian traces are affected by this regression.

Spike line generation fails because: the routine that selects spike line data from already found hover data uses point.kink which is only defined for scatter traces breaking the distance filter. As we don't have any spike tests with non-scatter traces, this went unnoticed.

So, we can easily add a fallback for that kink, but that's not enough to get back to the pre-1.33.0 behavior. In fact, this report highlights a somewhat fundamental problem with attributes hoverdistance and spikedistance: they don't work for features that use Fx.inbox to determine their hover data (e.g. bars, brick heatmaps and boxes). For thoses features, the point distance isn't the true cursor-to-data distance (for examples: see here and here). So, we can't just filter out points with distance greater than layout.hoverdistance and similarly for spikedistance.

Perhaps the easier way to fix this would be the state that hoverdistance and spikedistance don't apply to trace features that use Fx.inbox?

@alexcjohnson
Copy link
Collaborator

Working on this now...

Perhaps the easier way to fix this would be the state that hoverdistance and spikedistance don't apply to trace features that use Fx.inbox?

I think that's right - hoverdistance and spikedistance should only apply to point-like objects we're hovering on (and I guess line-like, when we implement that #1960). Hovering on areas - be they bars, boxes, scatter fills, heatmaps, contour maps... the rules I think I distill from our existing implementations of this are:

  • There's no "distance", you're either inside or outside the region.
  • Area hovers can't override point hovers.

Currently this is done by giving these areas a fictitious distance value that's bigger than MAXDIST (still treated as a constant, that needs to change now that hoverdistance is variable) but still returning the point. That's fine for hover, but not going to work as expected when this distance is reused for spikes. Also it seems like most of these area hovers should support spikes but some (I'm specifically thinking of scatter fills but perhaps there are others) should not. We'll have to be a little more careful about what these distances mean in order to support these cases.

@alexcjohnson
Copy link
Collaborator

Question: where should the spikes go for grouped bars? At the bar data position, or where this specific bar was shifted by the grouping?

Right now my code does either one, data position in compare mode and shifted position in closest mode (both of these are showing spikes for the brown bar in trace 5) but I think it should be consistent. I can see arguments for either one though - the data position is pointing to the real data value, and so links up better to other trace types (like scatter) and other subplots, so that's probably what I'd prefer, but the shifted position makes it clearer which bar the spike is for. Thoughts?
screen shot 2018-02-14 at 7 39 13 pm
screen shot 2018-02-14 at 7 39 42 pm

@etpinard
Copy link
Contributor

the data position is pointing to the real data value, and so links up better to other trace types (like scatter) and other subplots

I like this one better 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken regression this used to work
Projects
None yet
Development

No branches or pull requests

3 participants