Skip to content

Bar | histogram lasso / select box dragmode #2045

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.

Sign up for GitHub

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
Oct 2, 2017
Merged

Bar | histogram lasso / select box dragmode #2045

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e6bb32
add lasso/select box to bar traces
etpinard Sep 29, 2017
69d9c8c
add lasso/select box for histogram traces
etpinard Sep 29, 2017
3c73301
use toBeCloseTo for to assert numbers
etpinard Oct 2, 2017
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
1 change: 1 addition & 0 deletions src/traces/bar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Bar.arraysToCalcdata = require('./arrays_to_calcdata');
Bar.plot = require('./plot');
Bar.style = require('./style');
Bar.hoverPoints = require('./hover');
Bar.selectPoints = require('./select');

Bar.moduleType = 'trace';
Bar.name = 'bar';
Expand Down
17 changes: 12 additions & 5 deletions src/traces/bar/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,13 @@ module.exports = function plot(gd, plotinfo, cdbar) {
bartraces.append('g')
.attr('class', 'points')
.each(function(d) {
var t = d[0].t,
trace = d[0].trace,
poffset = t.poffset,
poffsetIsArray = Array.isArray(poffset);
var sel = d[0].node3 = d3.select(this);
var t = d[0].t;
var trace = d[0].trace;
var poffset = t.poffset;
var poffsetIsArray = Array.isArray(poffset);

d3.select(this).selectAll('g.point')
sel.selectAll('g.point')
.data(Lib.identity)
.enter().append('g').classed('point', true)
.each(function(di, i) {
Expand All @@ -69,12 +70,18 @@ module.exports = function plot(gd, plotinfo, cdbar) {
y1 = ya.c2p(p1, true);
x0 = xa.c2p(s0, true);
x1 = xa.c2p(s1, true);

// for selections
di.ct = [x1, (y0 + y1) / 2];
}
else {
x0 = xa.c2p(p0, true);
x1 = xa.c2p(p1, true);
y0 = ya.c2p(s0, true);
y1 = ya.c2p(s1, true);

// for selections
di.ct = [(x0 + x1) / 2, y1];
Copy link
Contributor Author

@etpinard etpinard Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

🆘 debatable logic alert 🆘

I chose to make a bar selected when the lasso/select polygon contains this point here which corresponds to the middle of the position coordinate at full size length.

Looking for 👍 s from @alexcjohnson @rreusser @monfera @chriddyp @cpsievert

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, that's a good question. The way you did it makes sense from a data perspective - the endpoint on the size axis is really the salient position of the bar's data, which is why we put the hover label there. But as far as the metaphor of "grabbing an object" goes, the center might be more intuitive. Usually I go with the data perspective though, and it seems to me like that might be more useful ("select all bars above 100" etc) and I bet people will get used to it. So I'm a 👍 but curious to hear others' perspectives - particularly @cpsievert

Copy link

Choose a reason for hiding this comment

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

That seems reasonable (especially with a 1-to-1 mapping from the (input) data value to bar), but another important question is what data do you attach to the select event when a single bar represents an aggregation of multiple values? It seems important to provide options as to how you'd like to respond to the event, so I if it were me, I would emit the raw data, the aggregated data, and the selection region (on the data scale).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I if it were me, I would emit the raw data, the aggregated data, and the selection region (on the data scale).

Right now, we emit raw data and selection region. Adding the aggregated data is an interesting idea.

Copy link

@cpsievert cpsievert Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

And, maybe I should point out that, assuming the model doesn't allow for partially selecting a bar that represents multiple values, I would actually vote for the selection region to cover the entire bar in order for a selection to be made...

The problem with that though is that it could be confusing for most users if we implement two different selection criteria based on whether an aggregation is taking place

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would offer a few arguments in favor of just keeping the "select the end" behavior:

  • Some users do their own aggregation, making a bar trace that conceptually represents a histogram. What would we do in that case, introduce an option for whether to select the end or the whole thing?
  • It would be meaningless (or at least ambiguous) to disaggregate by selecting a portion of the bar. Which sample is on top vs on bottom?
  • All our other selections work by collapsing the entity to a single point to be selected
  • It makes it that much harder to select just the bars you want: you could still select all the small bars, but you wouldn't have any way to do the likely more common act of selecting all the largest bars.

Copy link
Member

Choose a reason for hiding this comment

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

I would lean towards a consistent logic for bars & hists (and maybe the possibility of adding modes in the future). I think having a different behaviour across those two charts would cause more confusion than benefits across users, especially given how often I see users creating their own "histograms" with the bar trace as @alexcjohnson mentioned above.

Copy link
Contributor

@rreusser rreusser Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

More attributes are probably not a good idea, but perhaps the bar default would be data endpoints and the histogram default would be centroid. And if you dig deep enough to notice this subtlety, it'll lead to an attribute that can be toggled. If additional attributes had no cost, that'd be my preference, but additional attributes do have some small but nonzero cost (bundle size, documentation size, discoverability, maintenance, sprawl, configurability over good defaults, etc.) Realistically though, I would not actually expect the attribute to ever be toggled intentionally.

Copy link
Member

Choose a reason for hiding this comment

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

I think having a different behaviour across those two charts would cause more confusion than benefits across users

I agree. I think there should be one simple and predictable way for this to work.

If the app developer really wants to allow users to select "partial bars" because it represents important aggregated data, then I'd argue that they should be more explicit about this in their UI and draw a rug plot below their histogram and allow users to select the exact partial sets of points that way. Or even a box plot with points or eventually a violin plot with 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.

Ok. Thanks everyone for your input. I believe the keep-as-is side wins.

}

if(!isNumeric(x0) || !isNumeric(x1) ||
Expand Down
52 changes: 52 additions & 0 deletions src/traces/bar/select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/**
* Copyright 2012-2017, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

'use strict';

var DESELECTDIM = require('../../constants/interactions').DESELECTDIM;

module.exports = function selectPoints(searchInfo, polygon) {
var cd = searchInfo.cd;
var selection = [];
var trace = cd[0].trace;
var node3 = cd[0].node3;
var i;

if(trace.visible !== true) return;

if(polygon === false) {
// clear selection
for(i = 0; i < cd.length; i++) {
cd[i].dim = 0;
}
} else {
for(i = 0; i < cd.length; i++) {
var di = cd[i];

if(polygon.contains(di.ct)) {
selection.push({
pointNumber: i,
x: di.x,
y: di.y
});
di.dim = 0;
} else {
di.dim = 1;
}
}
}

node3.selectAll('.point').style('opacity', function(d) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This operation here is similar to what we currently do for scatter traces.

But note that, unlike scatter traces, bar traces do not have a marker.opacity attribute (see #1862) so currently, this here is equivalent to adding an alpha channel to marker.color, marker.line.color and textcolor.

cc #1848

return d.dim ? DESELECTDIM : 1;
});
node3.selectAll('text').style('opacity', function(d) {
return d.dim ? DESELECTDIM : 1;
});

return selection;
};
85 changes: 75 additions & 10 deletions test/jasmine/tests/select_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,11 +428,18 @@ describe('Test select box and lasso per trace:', function() {
var callNumber = 0;

return function(expected) {
var msg = '(call #' + callNumber + ') ';
var ranges = (selectedData.range || {})[subplot] || [];

expect(ranges)
.toBeCloseTo2DArray(expected, tol, msg + 'select box range for ' + subplot);
var msg = '(call #' + callNumber + ') select box range ';
var ranges = selectedData.range || {};

if(subplot) {
expect(ranges[subplot] || [])
.toBeCloseTo2DArray(expected, tol, msg + 'for ' + subplot);
} else {
expect(ranges.x || [])
.toBeCloseToArray(expected[0], tol, msg + 'x coords');
expect(ranges.y || [])
.toBeCloseToArray(expected[1], tol, msg + 'y coords');
}

callNumber++;
};
Expand All @@ -443,11 +450,18 @@ describe('Test select box and lasso per trace:', function() {
var callNumber = 0;

return function(expected) {
var msg = '(call #' + callNumber + ') ';
var lassoPoints = (selectedData.lassoPoints || {})[subplot] || [];

expect(lassoPoints)
.toBeCloseTo2DArray(expected, tol, msg + 'lasso points for ' + subplot);
var msg = '(call #' + callNumber + ') lasso points ';
var lassoPoints = selectedData.lassoPoints || {};

if(subplot) {
expect(lassoPoints[subplot] || [])
.toBeCloseTo2DArray(expected, tol, msg + 'for ' + subplot);
} else {
expect(lassoPoints.x || [])
.toBeCloseToArray(expected[0], tol, msg + 'x coords');
expect(lassoPoints.y || [])
.toBeCloseToArray(expected[1], tol, msg + 'y coords');
}

callNumber++;
};
Expand Down Expand Up @@ -708,4 +722,55 @@ describe('Test select box and lasso per trace:', function() {
.catch(fail)
.then(done);
}, LONG_TIMEOUT_INTERVAL);

it('should work for bar traces', function(done) {
var assertPoints = makeAssertPoints(['curveNumber', 'x', 'y']);
var assertRanges = makeAssertRanges();
var assertLassoPoints = makeAssertLassoPoints();

var fig = Lib.extendDeep({}, require('@mocks/0'));
fig.layout.dragmode = 'lasso';

Plotly.plot(gd, fig)
.then(function() {
return _run(
[[350, 200], [400, 200], [400, 250], [350, 250], [350, 200]],
function() {
assertPoints([
[0, 4.9, 0.371], [0, 5, 0.368], [0, 5.1, 0.356], [0, 5.2, 0.336],
[0, 5.3, 0.309], [0, 5.4, 0.275], [0, 5.5, 0.235], [0, 5.6, 0.192],
[0, 5.7, 0.145],
[1, 5.1, 0.485], [1, 5.2, 0.40900000000000003], [1, 5.3, 0.327],
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use toBeCloseTo above in makeAssertPoints and avoid 0.40900000000000003 etc?

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 3c73301

[1, 5.4, 0.24000000000000002], [1, 5.5, 0.149], [1, 5.6, 0.059],
[2, 4.9, 0.473], [2, 5, 0.36800000000000005], [2, 5.1, 0.258],
[2, 5.2, 0.14600000000000002], [2, 5.3, 0.03600000000000003]
]);
assertLassoPoints([
[4.87, 5.74, 5.74, 4.87, 4.87],
[0.53, 0.53, -0.02, -0.02, 0.53]
]);
},
null, LASSOEVENTS, 'bar lasso'
);
})
.then(function() {
return Plotly.relayout(gd, 'dragmode', 'select');
})
.then(function() {
return _run(
[[350, 200], [370, 220]],
function() {
assertPoints([
[0, 4.9, 0.371], [0, 5, 0.368], [0, 5.1, 0.356], [0, 5.2, 0.336],
[1, 5.1, 0.485], [1, 5.2, 0.40900000000000003],
[2, 4.9, 0.473], [2, 5, 0.36800000000000005]
]);
assertRanges([[4.87, 5.22], [0.31, 0.53]]);
},
null, BOXEVENTS, 'bar select'
);
})
.catch(fail)
.then(done);
});
});