Skip to content

Box points hover & select #2094

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 19 commits into from
Oct 19, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
13 changes: 12 additions & 1 deletion src/traces/box/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,5 +203,16 @@ module.exports = {
},
editType: 'plot'
},
fillcolor: scatterAttrs.fillcolor
fillcolor: scatterAttrs.fillcolor,
hoveron: {
valType: 'flaglist',
flags: ['boxes', 'points'],
dflt: 'boxes',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debatable choice here. Strictly speaking we need dflt: 'boxes' to keep backward compat, but perhaps someone can argue otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

dflt: 'boxes' 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree here, but perhaps to make this feature accessible to existing graphs, the show closest data on hover mode bar button could restyle hoveron to 'points+boxes' as well as layout.hovermode on click for traces with boxpoints: 'all'?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, hmm... that's a good point actually. Can I change my answer to dflt: 'boxes+points'? I don't think turning on a new hover feature really amounts to a breaking change, I suppose in principle it can lead to events that a user application doesn't know how to handle but that seems pretty minor IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

dflt: boxes+points 👍 One could even argue that not being able to hover on the box points is a 🐞 .

Copy link
Contributor Author

@etpinard etpinard Oct 18, 2017

Choose a reason for hiding this comment

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

done in 6bfcbe0

role: 'info',
editType: 'style',
description: [
'Do the hover effects highlight individual boxes ',
'or jitter points or both?'
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 call them jitter points - sample points?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in a1ea1e8

].join(' ')
}
};
2 changes: 2 additions & 0 deletions src/traces/box/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('marker.line.outlierwidth');
}
}

coerce('hoveron');
};
225 changes: 149 additions & 76 deletions src/traces/box/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,94 +14,167 @@ var Fx = require('../../components/fx');
var Color = require('../../components/color');

module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
// closest mode: handicap box plots a little relative to others
var cd = pointData.cd,
trace = cd[0].trace,
t = cd[0].t,
xa = pointData.xa,
ya = pointData.ya,
closeData = [],
dx, dy, distfn, boxDelta,
posLetter, posAxis,
val, valLetter, valAxis;

// adjust inbox w.r.t. to calculate box size
boxDelta = (hovermode === 'closest') ? 2.5 * t.bdPos : t.bdPos;

if(trace.orientation === 'h') {
dx = function(di) {
return Fx.inbox(di.min - xval, di.max - xval);
};
dy = function(di) {
var pos = di.pos + t.bPos - yval;
return Fx.inbox(pos - boxDelta, pos + boxDelta);
};
posLetter = 'y';
posAxis = ya;
valLetter = 'x';
valAxis = xa;
} else {
dx = function(di) {
var pos = di.pos + t.bPos - xval;
return Fx.inbox(pos - boxDelta, pos + boxDelta);
};
dy = function(di) {
return Fx.inbox(di.min - yval, di.max - yval);
};
posLetter = 'x';
posAxis = xa;
valLetter = 'y';
valAxis = ya;
}
var cd = pointData.cd;
var xa = pointData.xa;
var ya = pointData.ya;

var trace = cd[0].trace;
var hoveron = trace.hoveron;
var marker = trace.marker || {};

// output hover points array
var closeData = [];
// x/y/effective distance functions
var dx, dy, distfn;
// orientation-specific fields
var posLetter, valLetter, posAxis, valAxis;
// calcdata item
var di;
// hover point item extended from pointData
var pointData2;
// loop indices
var i, j;

if(hoveron.indexOf('boxes') !== -1) {
var t = cd[0].t;

// closest mode: handicap box plots a little relative to others
// adjust inbox w.r.t. to calculate box size
var boxDelta = (hovermode === 'closest') ? 2.5 * t.bdPos : t.bdPos;

if(trace.orientation === 'h') {
dx = function(di) {
return Fx.inbox(di.min - xval, di.max - xval);
};
dy = function(di) {
var pos = di.pos + t.bPos - yval;
return Fx.inbox(pos - boxDelta, pos + boxDelta);
};
posLetter = 'y';
posAxis = ya;
valLetter = 'x';
valAxis = xa;
} else {
dx = function(di) {
var pos = di.pos + t.bPos - xval;
return Fx.inbox(pos - boxDelta, pos + boxDelta);
};
dy = function(di) {
return Fx.inbox(di.min - yval, di.max - yval);
};
posLetter = 'x';
posAxis = xa;
valLetter = 'y';
valAxis = ya;
}

distfn = Fx.getDistanceFunction(hovermode, dx, dy);
Fx.getClosest(cd, distfn, pointData);

distfn = Fx.getDistanceFunction(hovermode, dx, dy);
Fx.getClosest(cd, distfn, pointData);
// skip the rest (for this trace) if we didn't find a close point
// and create the item(s) in closedata for this point
if(pointData.index !== false) {
di = cd[pointData.index];

// skip the rest (for this trace) if we didn't find a close point
if(pointData.index === false) return;
var lc = trace.line.color;
var mc = marker.color;

// create the item(s) in closedata for this point
if(Color.opacity(lc) && trace.line.width) pointData.color = lc;
else if(Color.opacity(mc) && trace.boxpoints) pointData.color = mc;
else pointData.color = trace.fillcolor;

// the closest data point
var di = cd[pointData.index],
lc = trace.line.color,
mc = (trace.marker || {}).color;
if(Color.opacity(lc) && trace.line.width) pointData.color = lc;
else if(Color.opacity(mc) && trace.boxpoints) pointData.color = mc;
else pointData.color = trace.fillcolor;
pointData[posLetter + '0'] = posAxis.c2p(di.pos + t.bPos - t.bdPos, true);
pointData[posLetter + '1'] = posAxis.c2p(di.pos + t.bPos + t.bdPos, true);

pointData[posLetter + '0'] = posAxis.c2p(di.pos + t.bPos - t.bdPos, true);
pointData[posLetter + '1'] = posAxis.c2p(di.pos + t.bPos + t.bdPos, true);
Axes.tickText(posAxis, posAxis.c2l(di.pos), 'hover').text;
pointData[posLetter + 'LabelVal'] = di.pos;

Axes.tickText(posAxis, posAxis.c2l(di.pos), 'hover').text;
pointData[posLetter + 'LabelVal'] = di.pos;
// box plots: each "point" gets many labels
var usedVals = {};
var attrs = ['med', 'min', 'q1', 'q3', 'max'];

// box plots: each "point" gets many labels
var usedVals = {},
attrs = ['med', 'min', 'q1', 'q3', 'max'],
attr,
pointData2;
if(trace.boxmean) attrs.push('mean');
if(trace.boxpoints) [].push.apply(attrs, ['lf', 'uf']);
if(trace.boxmean) attrs.push('mean');
if(trace.boxpoints) [].push.apply(attrs, ['lf', 'uf']);

for(var i = 0; i < attrs.length; i++) {
attr = attrs[i];
for(i = 0; i < attrs.length; i++) {
var attr = attrs[i];

if(!(attr in di) || (di[attr] in usedVals)) continue;
usedVals[di[attr]] = true;
if(!(attr in di) || (di[attr] in usedVals)) continue;
usedVals[di[attr]] = true;

// copy out to a new object for each value to label
val = valAxis.c2p(di[attr], true);
pointData2 = Lib.extendFlat({}, pointData);
pointData2[valLetter + '0'] = pointData2[valLetter + '1'] = val;
pointData2[valLetter + 'LabelVal'] = di[attr];
pointData2.attr = attr;
// copy out to a new object for each value to label
var val = valAxis.c2p(di[attr], true);
pointData2 = Lib.extendFlat({}, pointData);
pointData2[valLetter + '0'] = pointData2[valLetter + '1'] = val;
pointData2[valLetter + 'LabelVal'] = di[attr];
pointData2.attr = attr;

if(attr === 'mean' && ('sd' in di) && trace.boxmean === 'sd') {
pointData2[valLetter + 'err'] = di.sd;
if(attr === 'mean' && ('sd' in di) && trace.boxmean === 'sd') {
pointData2[valLetter + 'err'] = di.sd;
}
// only keep name on the first item (median)
pointData.name = '';

closeData.push(pointData2);
}
}
pointData.name = ''; // only keep name on the first item (median)
closeData.push(pointData2);
}

if(hoveron.indexOf('points') !== -1) {
var xPx = xa.c2p(xval);
var yPx = ya.c2p(yval);

// do not take jitter into consideration in compare hover modes
Copy link
Contributor Author

@etpinard etpinard Oct 17, 2017

Choose a reason for hiding this comment

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

Anyone disagrees here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, and might even take it farther - can we make all of the hover labels line up horizontally, even though the points do not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh hmm I guess https://github.com/plotly/plotly.js/pull/2094/files#r145223324 supersedes this comment, except perhaps if we show a point and stats in compare mode.

var kx, ky;
if(hovermode === 'closest') {
kx = 'x';
ky = 'y';
} else {
kx = 'xh';
ky = 'yh';
}

dx = function(di) {
var rad = Math.max(3, di.mrc || 0);
return Math.max(Math.abs(xa.c2p(di[kx]) - xPx) - rad, 1 - 3 / rad);
};
dy = function(di) {
var rad = Math.max(3, di.mrc || 0);
return Math.max(Math.abs(ya.c2p(di[ky]) - yPx) - rad, 1 - 3 / rad);
};
distfn = Fx.getDistanceFunction(hovermode, dx, dy);

for(i = 0; i < cd.length; i++) {
di = cd[i];

for(j = 0; j < (di.pts || []).length; j++) {
var pt = di.pts[j];

var newDistance = distfn(pt);
if(newDistance <= pointData.distance) {
pointData.distance = newDistance;

var xc = xa.c2p(pt.x, true);
var yc = ya.c2p(pt.y, true);
var rad = pt.mrc || 1;

pointData2 = Lib.extendFlat({}, pointData, {
// corresponds to index in x/y input data array
index: pt.i,
color: marker.color,
x0: xc - rad,
x1: xc + rad,
xLabelVal: pt.x,
y0: yc - rad,
y1: yc + rad,
yLabelVal: pt.y
});

closeData.push(pointData2);
}
}
}
}

return closeData;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another debatable choice: this commit allows 'boxes' and 'points' hoveron picks to coexist. With hoveron: 'boxes+points', we get:

peek 2017-10-16 23-51

where box and point hover label can be displayed at the same time. Does anyone find this potentially problematic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and under hovermode: 'x', this can happen:

peek 2017-10-17 00-05

Copy link
Collaborator

Choose a reason for hiding this comment

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

boxes+points could be useful, but I think we want to show fewer points. Currently it seems to return all the points that are in range, but most real-world box uses have far too many points to label them all, and we should match scatter which chooses the best point when more than one is in range.

So I guess I'm proposing:

  • In closest mode, show only one point or stats for one box, and points have priority
    • If there's a point in range and hoveron has points, show the best single point only.
    • If hoveron has boxes and there's no point in range (or hoveron doesn't have points), show the box stats.
  • In compare mode I don't really see any extra useful thing to do with points - I think we should still only show one point per trace, so I guess that means not considering only x when choosing the best point? I know that's not consistent with other types, but I feel like usability is much more important.
    • Perhaps though if hoveron = 'boxes+points' we should allow a point AND the box stats to be labeled?
    • Already if there are multiple boxes in range (ie boxmode = 'overlay') we'll see stats for all of them, right? I think that's good.

One other thought: do we want to do something to distinguish statistics labels from point labels? Perhaps actually say what statistical measure is being shown? Which might be nice on its own merits! But we'd have to be careful about the whisker ends, whether they represent upper/lower fence or max/min.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but most real-world box uses have far too many points to label them all, and we should match scatter

Good point here. I'll implement ⤴️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's what I come up with: f228a5e

One other thought: do we want to do something to distinguish statistics labels from point labels? Perhaps actually say what statistical measure is being shown?

The above commit does not implement this. I think it's a good idea though. Should the statistical measures always show up next to their value in the hover labels or just when point and box hover label show simultaneously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a quick-and-easy version:

peek 2017-10-19 12-02

we might have to expand out uf and lf. Spelling out upper fence and lower fence looks like:

peek 2017-10-19 12-06

does that look too cluttered to you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 for spelled out, I think you're right that uf / lf will confuse people.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe even spell out median

Copy link
Contributor Author

Choose a reason for hiding this comment

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

full version:

peek 2017-10-19 13-04

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in a3ec75a (with 🔨 🌴 commit b66628e)

};
11 changes: 7 additions & 4 deletions src/traces/box/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ module.exports = function plot(gd, plotinfo, cdbox) {
newJitter = trace.jitter * 2 / maxJitterFactor;
}

// fills in 'x' and 'y' in calcdata 'pts' item
// fills in 'x' and 'y' (with and w/o jitter offset) in calcdata 'pts' item
for(i = 0; i < pts.length; i++) {
var pt = pts[i];
var v = pt.v;
Expand All @@ -192,14 +192,17 @@ module.exports = function plot(gd, plotinfo, cdbox) {
bdPos * (newJitter * jitterFactors[i] * (rand() - 0.5)) :
0;

var posPx = d.pos + bPos + bdPos * trace.pointpos + jitterOffset;
var posPxNoJitter = d.pos + bPos + bdPos * trace.pointpos;
var posPx = posPxNoJitter + jitterOffset;

if(trace.orientation === 'h') {
pt.y = posPx;
pt.x = v;
pt.yh = posPxNoJitter;
pt.x = pt.xh = v;
} else {
pt.x = posPx;
pt.y = v;
pt.xh = posPxNoJitter;
pt.y = pt.yh = v;
}

// tag suspected outliers
Expand Down
42 changes: 42 additions & 0 deletions test/jasmine/tests/box_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,48 @@ describe('Test box hover:', function() {
nums: ['8.15', '0.75', '6.8', '10.25', '23.25', '5.25', '12'],
name: ['', '', '', '', '', '', ''],
axis: 'trace 0',
}, {
desc: 'hoveron points | hovermode closest',
patch: function(fig) {
fig.data.forEach(function(trace) {
trace.boxpoints = 'all';
trace.hoveron = 'points';
});
fig.layout.hovermode = 'closest';
return fig;
},
nums: ['(day 1, 0.7)', '(day 1, 0.6)', '(day 1, 0.6)'],
name: ['radishes', 'radishes', 'radishes']
}, {
desc: 'hoveron points | hovermode x',
patch: function(fig) {
fig.data.forEach(function(trace) {
trace.boxpoints = 'all';
trace.hoveron = 'points';
});
fig.layout.hovermode = 'x';
return fig;
},
nums: ['0', '0.3', '0.5', '0.6', '0.6', '0.7'],
name: ['radishes', 'radishes', 'radishes', 'radishes', 'radishes', 'radishes'],
axis: 'day 1'
}, {
desc: 'hoveron boxes+points | hovermode x',
patch: function(fig) {
fig.data.forEach(function(trace) {
trace.boxpoints = 'all';
trace.hoveron = 'points+boxes';
});
fig.layout.hovermode = 'x';
return fig;
},
nums: [
'0', '0.7', '0.6', '0.6', '0.5', '0.3', '0', '0.7', '0.6', '0.3', '0.55'
],
name: [
'', '', '', '', '', '', '', '', '', '', 'radishes'
],
axis: 'day 1'
}].forEach(function(specs) {
it('should generate correct hover labels ' + specs.desc, function(done) {
run(specs).catch(fail).then(done);
Expand Down