Skip to content

add 'width' to box and violin trace #3109

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

Closed
wants to merge 40 commits into from
Closed
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
8f04d13
added vwidth param - not working
Coding-with-Adam Oct 12, 2018
f76ca97
update dPos with vwidth//remove most console.logs
Coding-with-Adam Oct 15, 2018
81cdbe9
some revision based on review comments
Coding-with-Adam Oct 17, 2018
5ef2eeb
change dflt vwidth to 0;assign dPos for each trace
Coding-with-Adam Oct 18, 2018
1c6b26b
vwidth is now the half width // padding now uses trace.width values i…
Coding-with-Adam Oct 19, 2018
2d93a0e
vwidth to width
Coding-with-Adam Oct 19, 2018
1f9ced8
compact the t.dPos assignment with trace.width
Coding-with-Adam Oct 19, 2018
26f44c4
change location of width attribute description to be consistent with …
Coding-with-Adam Oct 19, 2018
77b9c9d
more complete descriptions
Coding-with-Adam Oct 19, 2018
243a369
add 1 image test
Coding-with-Adam Oct 22, 2018
eb5e9da
add new line to joyplot mock file
Coding-with-Adam Oct 22, 2018
eea273b
camelCase
Coding-with-Adam Oct 24, 2018
408aacb
zapped trailing ',' in box/violin attributes
Coding-with-Adam Oct 24, 2018
0555283
do not coerce scale{mode, group} if no width; otherwise set to dflt v…
Coding-with-Adam Oct 24, 2018
4eb8b44
first pass at calculating extreames for each trace
Coding-with-Adam Oct 24, 2018
44e0a88
fix up the syntax of violin/defaults
Coding-with-Adam Oct 24, 2018
f8b963b
remove old baseline png
Coding-with-Adam Oct 24, 2018
54aa355
generated updated joyplot baseline image
Coding-with-Adam Oct 24, 2018
0d4239b
remove padding depending on trace.side//regen baseline
Coding-with-Adam Oct 24, 2018
fa3a314
fix space syntax error
Coding-with-Adam Oct 24, 2018
c63780f
...
Coding-with-Adam Oct 31, 2018
c24330d
update algo for padding
Coding-with-Adam Nov 1, 2018
6e7883d
fix test syntax
Coding-with-Adam Nov 1, 2018
37786d6
remove consolve logs
Coding-with-Adam Nov 1, 2018
45ae47d
update kde line test
Coding-with-Adam Nov 1, 2018
8366c88
more testing with jasmine
Coding-with-Adam Nov 1, 2018
ce81377
remove commented lines
Coding-with-Adam Nov 1, 2018
8885b5e
remove another commented line and a useless loop
Coding-with-Adam Nov 1, 2018
57cc05a
remove maxHalfWidth var
Coding-with-Adam Nov 1, 2018
fe3d7ed
cleaner code for vpad assignment
Coding-with-Adam Nov 1, 2018
44fa269
inherit violin 'width' from box.width
etpinard Nov 6, 2018
f5ba38c
rm useless attrs in test mock
etpinard Nov 6, 2018
34722d3
mention that 'width' is in data coordinates
etpinard Nov 6, 2018
d1aed48
ignore box/violin *gap and *groupgap when 'width' is set
etpinard Nov 6, 2018
c70f7d5
do not coerce scale* attrs when violin 'width' is set
etpinard Nov 6, 2018
0dea0e9
Merge pull request #3222 from plotly/joyplots-etpinard
Kully Nov 7, 2018
5e5f9ee
vpadplus and minus dont cut off jitter:0 points on violin
Coding-with-Adam Nov 7, 2018
d04c60c
Merge branch 'joyplots' of https://github.com/plotly/plotly.js into j…
Coding-with-Adam Nov 7, 2018
91497e5
fix incorrect var name
Coding-with-Adam Nov 7, 2018
657848f
small syntax - infix operator spacing
Coding-with-Adam Nov 7, 2018
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
15 changes: 14 additions & 1 deletion src/traces/box/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,20 @@ module.exports = {
'the vertical (horizontal).'
].join(' ')
},

width: {
valType: 'number',
min: 0,
role: 'info',
dflt: 0,
editType: 'calc',
description: [
'Sets the width of the box.',
'If *0* (default value) the width is automatically selected based on the positions',
'of other box traces in the same subplot.',
].join(' ')
},

marker: {
outliercolor: {
valType: 'color',
Expand Down Expand Up @@ -244,7 +258,6 @@ module.exports = {
marker: scatterAttrs.unselected.marker,
editType: 'style'
},

hoveron: {
valType: 'flaglist',
flags: ['boxes', 'points'],
Expand Down
52 changes: 42 additions & 10 deletions src/traces/box/cross_trace_calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ var Lib = require('../../lib');

var orientations = ['v', 'h'];


function getPosition(di) {
return di.pos;
}

function crossTraceCalc(gd, plotinfo) {
var calcdata = gd.calcdata;
var xa = plotinfo.xaxis;
Expand Down Expand Up @@ -90,19 +95,46 @@ function setPositionOffset(traceType, gd, boxList, posAxis, pad) {
var groupgap = fullLayout[traceType + 'groupgap'];
var padfactor = (1 - gap) * (1 - groupgap) * dPos / fullLayout[numKey];

// autoscale the x axis - including space for points if they're off the side
// TODO: this will overdo it if the outermost boxes don't have
// their points as far out as the other boxes
var extremes = Axes.findExtremes(posAxis, boxdv.vals, {
vpadminus: dPos + pad[0] * padfactor,
vpadplus: dPos + pad[1] * padfactor
});
// Find maximum trace width
// we baseline this at dPos
var maxHalfWidth = dPos;
for(i = 0; i < boxList.length; i++) {
calcTrace = calcdata[boxList[i]];

if(calcTrace[0].trace.width) {
if(calcTrace[0].trace.width / 2 > maxHalfWidth) {
maxHalfWidth = calcTrace[0].trace.width / 2;
}
}
}

for(i = 0; i < boxList.length; i++) {
calcTrace = calcdata[boxList[i]];
// set the width of all boxes
calcTrace[0].t.dPos = dPos;
// link extremes to all boxes
// set the width of this box
// override dPos with trace.width if present
var thisDPos = calcTrace[0].t.dPos = (calcTrace[0].trace.width / 2) || dPos;
var positions = calcTrace.map(getPosition);
// autoscale the x axis - including space for points if they're off the side
// TODO: this will overdo it if the outermost boxes don't have
// their points as far out as the other boxes
var vpadminus = 0;
var vpadplus = 0;
if(calcTrace[0].trace.side === 'positive') {
vpadminus = 0;
vpadplus = thisDPos + pad[1] * padfactor;
} else if(calcTrace[0].trace.side === 'negative') {
vpadminus = thisDPos + pad[0] * padfactor;
vpadplus = 0;
} else {
// if side === 'both'
vpadminus = thisDPos + pad[0] * padfactor;
vpadplus = thisDPos + pad[1] * padfactor;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

🌴

var side = calcTrace[0].trace.side;
var vpadminus = (side === 'positive') ? 0 : (thisDPos + pad[0] * padfactor);
var vpadplus = (side === 'negative') ? 0 : (thisDPos + pad[1] * padfactor);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit worried actually that this will exacerbate the issue in the TODO above. For example what if you have a single-sided violin but the points get drawn on the other side? It looks to me like this algorithm will cut them off entirely. @etpinard thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does 🌴 mean? Don't repeat yourself?

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'm a bit worried actually that this will exacerbate the issue in the TODO above

I noticed another bug - if we choose to show the box, it gets cut in half if we choose positive or negative for side
half violin

DATA:

var data = [{
    type: 'violin',
    x: [0, 5, 7, 8],
    points: 'all',
    side: 'positive',
    box: {
      visible: true
    },
    boxpoints: true,
    line: {
      color: 'black'
    },
    fillcolor: '#8dd3c7',
    opacity: 0.6,
    meanline: {
      visible: true
    },
    y0: 0.0
}];

I think we are going to have to correlate the currently 0 vpad with the span of the box and visible points.

The fact that the image tests passed tells me an example like ^ is not in mocks. I think we should add a single sided violin baseline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What does 🌴 mean? Don't repeat yourself?

yeah sorry, DRY - I guess that's just a plotly.js convention.

I noticed another bug - if we choose to show the box, it gets cut in half if we choose positive or negative for side

That's intentional, eg for two back-to-back violins. https://github.com/plotly/plotly.js/blob/master/test/image/baselines/violin_side-by-side.png


var extremes = Axes.findExtremes(posAxis, positions, {
vpadminus: vpadminus,
vpadplus: vpadplus
});
calcTrace[0].trace._extremes[posAxis._id] = extremes;
}

Expand Down
1 change: 1 addition & 0 deletions src/traces/box/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ function supplyDefaults(traceIn, traceOut, defaultColor, layout) {

coerce('whiskerwidth');
coerce('boxmean');
coerce('width');

var notched = coerce('notched', traceIn.notchwidth !== undefined);
if(notched) coerce('notchwidth');
Expand Down
14 changes: 14 additions & 0 deletions src/traces/violin/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,20 @@ module.exports = {
'right (left) for vertical violins and above (below) for horizontal violins.'
].join(' ')
}),

width: {
valType: 'number',
min: 0,
role: 'info',
dflt: 0,
editType: 'calc',
description: [
'Sets the width of the violin.',
'If *0* (default value) the width is automatically selected based on the positions',
'of other violin traces in the same subplot.',
].join(' ')
},

marker: boxAttrs.marker,
text: boxAttrs.text,

Expand Down
10 changes: 8 additions & 2 deletions src/traces/violin/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,15 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(traceOut.visible === false) return;

coerce('bandwidth');
coerce('scalegroup', traceOut.name);
coerce('scalemode');
coerce('side');
var width = coerce('width');
if(!width) {
coerce('scalegroup', traceOut.name);
coerce('scalemode');
} else {
traceOut.scalegroup = '';
traceOut.scalemode = 'width';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does anything break if we 🔪 the else entirely? Unnecessary attributes should simply not be included in traceOut at all.

Copy link
Contributor Author

@Kully Kully Nov 1, 2018

Choose a reason for hiding this comment

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

yes, the violins are not drawn properly: the outlines are filled with black ink


var span = coerce('span');
var spanmodeDflt;
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
64 changes: 64 additions & 0 deletions test/image/mocks/violin_box_multiple_widths.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
{
"data": [{
"type": "violin",
"width": 0.315,
"x": [0, 5, 7, 8],
"points": "none",
"side": "positive",
"box": {
"visible": false
},
"boxpoints": false,
"line": {
"color": "black"
},
"fillcolor": "#8dd3c7",
"opacity": 0.6,
"meanline": {
"visible": false
},
"y0": 0.0
}, {
"type": "violin",
"x": [0, 5, 7, 8],
"points": "none",
"side": "positive",
"box": {
"visible": false
},
"boxpoints": false,
"line": {
"color": "black"
},
"fillcolor": "#d3c78d",
"opacity": 0.6,
"meanline": {
"visible": false
},
"y0": 0.1
}, {
"type": "box",
"width": 0.5421,
"x": [0, 5, 7, 8],
"points": "none",
"side": "positive",
"box": {
"visible": false
},
"boxpoints": false,
"line": {
"color": "black"
},
"fillcolor": "#c78dd3",
"opacity": 0.6,
"meanline": {
"visible": false
},
"y0": 0.2
}],
"layout": {
"title": "Joyplot - Violin with multiple widths",
"xaxis": {"zeroline": false},
"violingap": 0
}
}
5 changes: 3 additions & 2 deletions test/jasmine/tests/violin_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ describe('Test violin hover:', function() {
Plotly.plot(gd, fig).then(function() {
mouseEvent('mousemove', 250, 250);
assertViolinHoverLine([299.35, 250, 200.65, 250]);
// assertViolinHoverLine([178.67823028564453, 250, 80, 250]);
})
.catch(failTest)
.then(done);
Expand All @@ -533,7 +534,7 @@ describe('Test violin hover:', function() {

Plotly.plot(gd, fig).then(function() {
mouseEvent('mousemove', 300, 250);
assertViolinHoverLine([299.35, 250, 250, 250]);
assertViolinHoverLine([178.67823028564453, 250, 80, 250]);
Copy link
Contributor

@etpinard etpinard Nov 7, 2018

Choose a reason for hiding this comment

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

@Kully do you know why this test had to be modified?

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 wasn't too sure to be honest. I ran the test a few times but there were only two values the assert was saying was valid, iirc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably it changed because autorange can shrink depending on side now, so the violin moved.

})
.catch(failTest)
.then(done);
Expand All @@ -544,7 +545,7 @@ describe('Test violin hover:', function() {

Plotly.plot(gd, fig).then(function() {
mouseEvent('mousemove', 200, 250);
assertViolinHoverLine([200.65, 250, 250, 250]);
assertViolinHoverLine([321.3217315673828, 250, 420, 250]);
})
.catch(failTest)
.then(done);
Expand Down