Skip to content

fix for spikedistance=-1 #4637

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

Merged
merged 3 commits into from
Mar 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/fx/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,7 @@ function _hover(gd, evt, subplot, noHoverEvent) {
var thisSpikeDistance;
for(var i = 0; i < pointsData.length; i++) {
thisSpikeDistance = pointsData[i].spikeDistance;
if(thisSpikeDistance < minDistance && thisSpikeDistance <= spikedistance) {
if(thisSpikeDistance <= minDistance && thisSpikeDistance <= spikedistance) {
resultPoint = pointsData[i];
minDistance = thisSpikeDistance;
}
Expand Down
3 changes: 1 addition & 2 deletions src/traces/bar/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ function hoverOnBars(pointData, xval, yval, hovermode) {
var isClosest = (hovermode === 'closest');
var isWaterfall = (trace.type === 'waterfall');
var maxHoverDistance = pointData.maxHoverDistance;
var maxSpikeDistance = pointData.maxSpikeDistance;

var posVal, sizeVal, posLetter, sizeLetter, dx, dy, pRangeCalc;

Expand Down Expand Up @@ -161,7 +160,7 @@ function hoverOnBars(pointData, xval, yval, hovermode) {
pointData.valueLabel = hoverLabelText(sa, pointData[sizeLetter + 'LabelVal']);

// spikelines always want "closest" distance regardless of hovermode
pointData.spikeDistance = (sizeFn(di) + thisBarPositionFn(di)) / 2 + maxSpikeDistance - maxHoverDistance;
pointData.spikeDistance = (sizeFn(di) + thisBarPositionFn(di)) / 2 - maxHoverDistance;
Copy link
Contributor Author

@antoinerg antoinerg Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxHoverDistance is necessary here because it is added in the function sizeFn. Now, why would a function computing the size of a bar would contain maxHoverDistance is beyond my comprehension.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, super confusing. It's not computing the size of a bar, it's computing a distance measure along the size axis. Is it necessary to remove maxSpikeDistance here? Or it just doesn't have an understood purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to remove maxSpikeDistance here?

Keeping maxSpikeDistance in leads to:
Screenshot_2020-03-13_15-47-32

Or it just doesn't have an understood purpose?

I'll be honest: I also don't understand its purpose. What is spikedistance supposed to represent? Why would we add maxSpikeDistance and substract maxHoverDistance to it at the trace-level? It seems to me that this logic should live in fx/hover.js no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this logic should certainly be cleaned up at some point... super hacky. Perhaps as part of #4638 🙏

// they also want to point to the data value, regardless of where the label goes
// in case of bars shifted within groups
pointData[posLetter + 'Spike'] = pa.c2p(di.p, true);
Expand Down
158 changes: 158 additions & 0 deletions test/jasmine/tests/hover_spikeline_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,29 @@ describe('spikeline hover', function() {
.then(done);
});

it('correctly select the closest bar even when setting spikedistance to -1', function(done) {
var mock = require('@mocks/bar_stack-with-gaps');
var mockCopy = Lib.extendDeep({}, mock);
mockCopy.layout.xaxis.showspikes = true;
mockCopy.layout.yaxis.showspikes = true;
mockCopy.layout.spikedistance = -1;

Plotly.newPlot(gd, mockCopy)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking:
We could us newPlot > plot here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that desirable? plot is not part of the API we want anyone using, so I'd kind of rather not use it in tests either, even if it's a little faster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the note. Resolving...
And please feel free to open an issue if you prefer to chage similar cases in all the tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't bother converting the rest unless there are problems. Sometimes there's a good reason to go for the faster one, in particular I seem to recall a lot of overhead to newPlot in WebGL tests? But that overhead comes with more assurance that there's no cross-test interaction, which has lead to hard-to-debug errors in the past. @etpinard any words of wisdom to add here?

.then(function() {
_hover({xpx: 600, ypx: 400});
var lines = d3.selectAll('line.spikeline');
expect(lines.size()).toBe(4);
expect(lines[0][1].getAttribute('stroke')).toBe('#2ca02c');

_hover({xpx: 600, ypx: 200});
lines = d3.selectAll('line.spikeline');
expect(lines.size()).toBe(4);
expect(lines[0][1].getAttribute('stroke')).toBe('#1f77b4');
})
.catch(failTest)
.then(done);
});

it('correctly responds to setting the spikedistance to 0 by disabling ' +
'the search for points to draw the spikelines', function(done) {
var _mock = makeMock('toaxis', 'closest');
Expand Down Expand Up @@ -593,4 +616,139 @@ describe('spikeline hover', function() {
.catch(failTest)
.then(done);
});

it('correctly draws lines up to the last point', function(done) {
Plotly.newPlot(gd, [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking:
We could us newPlot > plot here.

{type: 'bar', y: [5, 7, 9, 6, 4, 3]},
{y: [5, 7, 9, 6, 4, 3]},
{y: [5, 7, 9, 6, 4, 3], marker: {color: 'red'}}
], {
xaxis: {showspikes: true},
yaxis: {showspikes: true},
spikedistance: -1,
width: 400, height: 400,
showlegend: false
})
.then(function() {
_hover({xpx: 150, ypx: 250});

var lines = d3.selectAll('line.spikeline');
expect(lines.size()).toBe(4);
expect(lines[0][1].getAttribute('stroke')).toBe('red');
expect(lines[0][3].getAttribute('stroke')).toBe('red');
})
.catch(failTest)
.then(done);
});

describe('works across all cartesian traces', function() {
var schema = Plotly.PlotSchema.get();
var traces = Object.keys(schema.traces);
var tracesSchema = [];
var i, j, k;
for(i = 0; i < traces.length; i++) {
tracesSchema.push(schema.traces[traces[i]]);
}
var excludedTraces = [ 'image' ];
var cartesianTraces = tracesSchema.filter(function(t) {
return t.categories.length &&
t.categories.indexOf('cartesian') !== -1 &&
t.categories.indexOf('noHover') === -1 &&
excludedTraces.indexOf(t.type) === -1;
});

function makeData(type, axName, a, b) {
var input = [a, b];
var cat = input[axName === 'yaxis' ? 1 : 0];
var data = input[axName === 'yaxis' ? 0 : 1];

var measure = [];
for(j = 0; j < data.length; j++) {
measure.push('absolute');
}

var z = Lib.init2dArray(cat.length, data.length);
for(j = 0; j < z.length; j++) {
for(k = 0; k < z[j].length; k++) {
z[j][k] = 0;
}
}
if(axName === 'xaxis') {
for(j = 0; j < b.length; j++) {
z[0][j] = b[j];
}
}
if(axName === 'yaxis') {
for(j = 0; j < b.length; j++) {
z[j][0] = b[j];
}
}

return Lib.extendDeep({}, {
orientation: axName === 'yaxis' ? 'h' : 'v',
type: type,
x: cat,
a: cat,

b: data,
y: data,
z: z,

// For OHLC
open: data,
close: data,
high: data,
low: data,

// For histogram
nbinsx: cat.length,
nbinsy: data.length,

// For waterfall
measure: measure,

// For splom
dimensions: [
{
label: 'DimensionA',
values: a
},
{
label: 'DimensionB',
values: b
}
]
});
}

cartesianTraces.forEach(function(trace) {
it('correctly responds to setting the spikedistance to -1 for ' + trace.type, function(done) {
var type = trace.type;
var x = [4, 5, 6];
var data = [7, 2, 3];

var mock = {
data: [makeData(type, 'xaxis', x, data)],
layout: {
spikedistance: -1,
xaxis: {showspikes: true},
yaxis: {showspikes: true},
zaxis: {showspikes: true},
title: {text: trace.type},
width: 400, height: 400
}
};

Plotly.newPlot(gd, mock)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking:
We could us newPlot > plot here.

.then(function() {
_hover({xpx: 200, ypx: 100});

var lines = d3.selectAll('line.spikeline');
expect(lines.size()).toBe(4);
})
.catch(failTest)
.then(done);
});
});
});
});