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

add 'width' to box and violin trace #3109

Closed
wants to merge 40 commits into from
Closed
Show file tree
Hide file tree
Changes from 11 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
18 changes: 16 additions & 2 deletions 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 All @@ -255,5 +268,6 @@ module.exports = {
'Do the hover effects highlight individual boxes ',
'or sample points or both?'
].join(' ')
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

⚡ that ,

Not sure why our linting didn't pick this up.

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 added this as I placed the width property at the end of this list the first time around. I then moved to the middle to a more appropriate spot but forgot to ⚡️ the ,


};
21 changes: 18 additions & 3 deletions src/traces/box/cross_trace_calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,18 +90,33 @@ function setPositionOffset(traceType, gd, boxList, posAxis, pad) {
var groupgap = fullLayout[traceType + 'groupgap'];
var padfactor = (1 - gap) * (1 - groupgap) * dPos / fullLayout[numKey];

// Find maximum trace width
// we baseline this at dPos
var max_half_width = dPos;
Copy link
Contributor

Choose a reason for hiding this comment

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

Camel case please 🐫

for(i = 0; i < boxList.length; i++) {
calcTrace = calcdata[boxList[i]];

if(calcTrace[0].trace.width) {
if(calcTrace[0].trace.width / 2 > max_half_width) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. What if trace.width is smaller than the minimum-diff-between-distinct-values? Does this still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually does work. See this line where the width is set.

These lines are affecting the axis autoscale of the plotted points. But if a violin has trace.width = 0.00001 (much less than the min-diff as you mention) then that trace will shrink to the whatever the width is set to but the "camera" will not zoom in anymore. The view of all the traces is always calibrated to the violin/box with the maximum width set, which doesn't loose any desired layout that you could want with violins or boxes, if you decide to use width at all.

max_half_width = calcTrace[0].trace.width / 2;
}
}
}
alexcjohnson marked this conversation as resolved.
Show resolved Hide resolved

// 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
vpadminus: max_half_width + pad[0] * padfactor,
vpadplus: max_half_width + 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.

Ah good catch - unfortunately the TODO concern above this gets more acute with custom widths. If you show two traces, one off to the left with a very narrow width and a wide one to the right, the one on the left will be padded as much as the one on the right, giving an inappropriately large autorange.

It looks to me as though we could pretty easily fix this by calculating extremes per trace. That would go below in the trace loop and might be as simple as:

var thisDPos = calcTrace[0].t.dPos = (calcTrace[0].trace.width / 2) || dPos;
var positions = calcTrace.map(function(di) { return di.pos });
var extremes = Axes.findExtremes(posAxis, positions, {
    vpadminus: thisDPos + pad[0] * padfactor,
    vpadminus: thisDPos + pad[1] * padfactor
});
calcTrace[0].trace._extremes[posAxis._id] = extremes;

That wouldn't address the original TODO - which I guess is about a per-trace pad / padfactor depending on trace.pointpos, I'd have to look to see if there's an easy solution to that, there may be... but if you don't feel like digging into that feel free to just move the TODO along with this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For

  violin1 => x0=0.0, width=0.1
  violin2 => x0=0.1, width=1.0

with changes

and PRE your change above:

before change

Copy link
Contributor Author

@Kully Kully Oct 24, 2018

Choose a reason for hiding this comment

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

not that much improvement, but there is some. Notice the vertical distance between the -0.4 and the -5 label. We need a better algo for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right - because we don't take trace.side into account - if it's 'positive' we should set vpadminus: 0, if 'negative' we should set vpadplus: 0.

Copy link
Contributor Author

@Kully Kully Oct 24, 2018

Choose a reason for hiding this comment

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

muuuch better:

newplot

});

for(i = 0; i < boxList.length; i++) {
calcTrace = calcdata[boxList[i]];
// set the width of all boxes
calcTrace[0].t.dPos = dPos;
// override dPos with trace.width if present
calcTrace[0].t.dPos = (calcTrace[0].trace.width / 2) || dPos;

// link extremes to all boxes
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
17 changes: 16 additions & 1 deletion 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 Expand Up @@ -243,5 +257,6 @@ module.exports = {
'Do the hover effects highlight individual violins',
'or sample points or the kernel density estimate or any combination of them?'
].join(' ')
}
},

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

coerce('bandwidth');
coerce('side');
coerce('scalegroup', traceOut.name);
coerce('scalemode');
coerce('side');
coerce('width');

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
}
}