add 'width' to box and violin trace #3109

Kully · 2018-10-15T23:33:09Z

re: plotly/documentation#1090
inspiration for joyplots found in community discussion here: https://community.plot.ly/t/ridgeline-joy-plots-in-dash-automatic/12602

First PR for js, whooho 🎉

This is my first shot at adding a width parameter for violin and box (I am calling the param vwidth for now). So far it works okay. The only thing I am noticing is that

all traces in the data object have to be set to the same value in order for there to be any update to the plot

Running this script

function violin(y0, fillcolor, vwidth) {
    var v = {
      type: 'violin',
      vwidth: vwidth,
      x: [0, 5, 7, 8],
      points: 'none',
      side: 'positive',
      box: {
        visible: false
      },
      boxpoints: false,
      line: {
        color: 'black'
      },
      fillcolor: fillcolor,
      opacity: 0.6,
      meanline: {
        visible: false
      },
      y0: y0
    };
    return v
};

var data = [
  violin(0.0, '#8dd3c7', 0.4),
  violin(0.1, '#d3c78d', 0.4),
  violin(0.2, '#c78dd3', 0.4),
];

var layout = {
  title: "Horizontal JoyPlot",
  xaxis: {
    zeroline: false
  },
  violingap: 0
};

Plotly.newPlot('graph', data, layout);

I get:

and tweaking

var data = [
  violin(0.0, '#8dd3c7', 1),
  violin(0.1, '#d3c78d', 1),
  violin(0.2, '#c78dd3', 1),
];

to

var data = [
  violin(0.0, '#8dd3c7', 1),
  violin(0.1, '#d3c78d', 0.4),
  violin(0.2, '#c78dd3', 1),
];

I get the following:

And changing the snippet to

var data = [
  violin(0.0, '#8dd3c7', 0.4),
  violin(0.1, '#d3c78d', 0.4),
  violin(0.2, '#c78dd3', 0.4),
];

where all the vwidths are 0.4, we get what we should be getting.

I am contemplating switching vwidth to a layout attribute in the same category as violingap. I would appreciate some input before continuing.

cc @etpinard

alexcjohnson · 2018-10-16T21:14:55Z

Welcome to plotly.js @Kully 🎉 Thanks for the PR!

Lets call this parameter just width like we do in bar traces, and keep it in the trace, not in layout. And we might as well allow it for both box and violin traces.

all traces in the data object have to be set to the same value in order for there to be any update to the plot

I suspect in fact all that matters as you have it now is the last trace (of the right type on the same subplot). That's because you're taking trace.vwidth, where trace is just the last one in the loop above, and applying it to the whole boxList (or violinList) of traces. What you should be doing instead is setting this per-trace down here by checking if calcTrace[0].trace exists and using it instead of dPos. (BTW factor of 2? dPos is the half width, and in your example there's only one half of the violin, but in general I think we want this to be the full width even if you're only showing half; and definitely full width for box)

src/plots/plots.js

alexcjohnson · 2018-10-16T21:23:40Z

src/traces/box/attributes.js

+        valType: 'number',
+        min: 0,
+        role: 'info',
+        dflt: false,


I might just set the dflt to 0 and have that mean "auto". That's a convention we use various other places. Then you can (still) use truthiness test if(trace.width) to tell if the user has specified an explicit width.

BTW this is in box/attributes and you have explicitly disabled this for boxes below but I'm sure you'll enable it in a revision 😁

(also this is in box/attributes but the description below says violins...)

That's a convention we use various other places.

Hmm. Bar width has dflt: null which I think is more appropriate

plotly.js/src/traces/bar/attributes.js

Lines 155 to 165 in 7ae6fd6

width: {

valType: 'number',

dflt: null,

min: 0,

arrayOk: true,

role: 'info',

editType: 'calc',

description: [

'Sets the bar width (in position axis units).'

].join(' ')

},

Which "other places" are you referring to?

I was thinking of nticks, nbins(x|y), also looks like we do that for maxdisplayed.

null is certainly more meaningful on its own than 0, and it's necessary if 0 is a valid value distinct from "auto" - like bar.offset where 0 means "center the bar at the data value" and "auto" means "if the bars are grouped, offset them all so they end up side-by-side".

It's just a little funny though with valType: 'number' - it means null looks invalid, so if you try and set it we'll determine it to be invalid and fall back on the default, which I guess is fine as long as null is the default, but it would still fail Plotly.validate. Also you can't set it at all via restyle/relayout, because null there means "unset" - again, not immediately a problem as long as this is the default. BUT... what if you have a template that overrides the default? Seems to me then you would be unable to revert to auto with any setting at all of the user attribute, right?

valType: 'angle' adds a special value 'auto', which might be the best of both worlds? Its meaning is clear (and distinct from 0), and it's not intercepted by plotlyjs like null is. Perhaps we should allow valType: 'number' to specify allowAuto: 'true', or extras: ['auto']?

BUT... what if you have a template that overrides the default? Seems to me then you would be unable to revert to auto with any setting at all of the user attribute, right?

Interesting point. This is somewhat related to #2834, I'll comment over there.

alexcjohnson · 2018-10-16T21:27:39Z

src/traces/violin/defaults.js

@@ -29,6 +29,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
    coerce('scalegroup', traceOut.name);
    coerce('scalemode');
    coerce('side');
+    coerce('vwidth');


I suspect we should disable scalegroup and scalemode if there's a width... otherwise it would be very confusing what should happen. That would look like:

var width = coerce('width'); if(!width) { coerce('scalegroup', traceOut.name); coerce('scalemode'); }

scalegroup and scalecount seem to not do anything when there is width

Exactly, that's why we don't want to coerce them - every attribute that makes it to traceOut should do something.

Oh sorry, I marked this resolved when I saw that you added the conditional coerce block... but I guess you removed it again in a later commit. We do want the conditional, per my previous comment.

src/traces/box/cross_trace_calc.js

src/traces/box/attributes.js

src/traces/box/cross_trace_calc.js

alexcjohnson · 2018-10-19T14:36:16Z

@Kully re: tests - I think all we need for this feature is one or two image tests - covering both box and violin, with at least two different explicit widths and one automatic width on the same subplot.

…f not 0

…other trace types

alexcjohnson · 2018-10-19T17:17:34Z

src/traces/box/cross_trace_calc.js

-        vpadminus: dPos + pad[0] * padfactor,
-        vpadplus: dPos + pad[1] * padfactor
+        vpadminus: max_half_width + pad[0] * padfactor,
+        vpadplus: max_half_width + pad[1] * padfactor


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.

For

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

and PRE your change above:

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.

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.

muuuch better:

Kully · 2018-10-22T23:27:42Z

@alexcjohnson image test is done. anything else that needs updating/adding before merging this guy?

etpinard · 2018-10-23T18:38:10Z

src/traces/box/cross_trace_calc.js

@@ -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;


Camel case please 🐫

etpinard · 2018-10-23T18:47:06Z

src/traces/box/attributes.js

@@ -255,5 +268,6 @@ module.exports = {
            'Do the hover effects highlight individual boxes ',
            'or sample points or both?'
        ].join(' ')
-    }
+    },


⚡ that ,

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

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 ,

etpinard · 2018-10-23T18:50:09Z

src/traces/box/cross_trace_calc.js

+        calcTrace = calcdata[boxList[i]];
+
+        if(calcTrace[0].trace.width) {
+            if(calcTrace[0].trace.width / 2 > max_half_width) {


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

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.

test/jasmine/tests/violin_test.js

alexcjohnson · 2018-11-01T22:54:55Z

src/traces/violin/defaults.js

+    } else {
+        traceOut.scalegroup = '';
+        traceOut.scalemode = 'width';
+    }


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

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

etpinard · 2018-11-06T21:17:30Z

Hmm. Looks like {violin|box}gap and {violin|box}groupgap still have an effect here when width is set. Meaning width corresponds to the box/violin width only when *gap and *groupgap are set to 0.

Is this desired behavior? Or should we do like bar traces and ignore *gap and *groupgap attributes whenever a user sets width?

- do not fill in fullLayout._violinScaleGroupStats for traces with set 'width' - use maxKDE key instead of maxWidth to avoid confusing

etpinard · 2018-11-06T22:09:45Z

Or should we do like bar traces and ignore *gap and *groupgap attributes whenever a user sets width?

I implemented this along with fixing #3109 (comment) and improving the attribute descriptions in ->

joyplots...joyplots-etpinard

alexcjohnson · 2018-11-06T22:27:54Z

Or should we do like bar traces and ignore *gap and *groupgap attributes whenever a user sets width?

💯 good 👀 you're absolutely correct.

Kully · 2018-11-06T23:01:07Z

I implemented this along with fixing #3109 (comment) and improving the attribute descriptions in ->

Nice stuff!

Kully · 2018-11-07T13:30:28Z

I implemented this along with fixing #3109 (comment) and improving the attribute descriptions in ->

joyplots...joyplots-etpinard

I gave this a quick scan - if you are fixing the coercing issue with these two variables, I'm good with that. Looks good to merge in.

Joyplots etpinard

etpinard · 2018-11-07T14:37:16Z

test/jasmine/tests/violin_test.js

@@ -533,7 +550,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]);


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

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.

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

Kully · 2018-11-07T15:40:43Z

You can get a cool effect by stacking violins with big widths on top of each other:

…oyplots merging in Etienne's edits

Kully · 2018-11-07T18:10:36Z

I think I have solved the points getting cut off issue except for when jitter is other than 0. Now points that are below a positive trace are in the range, but right at their midpoints. Same goes with negative violin traces:

What I see left is to:

correct for jitter != 0
uncover the rest of the markers by using something like trace.marker.size. How can I find the actual displayed height in pixels of the markers in the chart?
add a little extra padding to go a little past the dots, just to make it look closer to the older method

Does this seem like the right direction to you guys?

etpinard · 2018-11-12T14:16:02Z

Now in #3234

Coding-with-Adam added 2 commits October 12, 2018 14:55

added vwidth param - not working

8f04d13

update dPos with vwidth//remove most console.logs

f76ca97

alexcjohnson reviewed Oct 16, 2018

View reviewed changes

src/plots/plots.js Outdated Show resolved Hide resolved

alexcjohnson reviewed Oct 16, 2018

View reviewed changes

src/traces/box/cross_trace_calc.js Outdated Show resolved Hide resolved

some revision based on review comments

81cdbe9

alexcjohnson added feature something new status: in progress labels Oct 17, 2018

change dflt vwidth to 0;assign dPos for each trace

5ef2eeb

alexcjohnson reviewed Oct 19, 2018

View reviewed changes

src/traces/box/attributes.js Outdated Show resolved Hide resolved

alexcjohnson reviewed Oct 19, 2018

View reviewed changes

src/traces/box/cross_trace_calc.js Outdated Show resolved Hide resolved

Coding-with-Adam added 4 commits October 19, 2018 10:47

vwidth is now the half width // padding now uses trace.width values i…

1c6b26b

…f not 0

vwidth to width

2d93a0e

compact the t.dPos assignment with trace.width

1f9ced8

change location of width attribute description to be consistent with …

26f44c4

…other trace types

alexcjohnson reviewed Oct 19, 2018

View reviewed changes

Coding-with-Adam added 3 commits October 19, 2018 17:10

more complete descriptions

77b9c9d

add 1 image test

243a369

add new line to joyplot mock file

eb5e9da

Kully added status: reviewable and removed status: in progress labels Oct 23, 2018

etpinard reviewed Oct 23, 2018

View reviewed changes

alexcjohnson mentioned this pull request Oct 24, 2018

2951 contrasting pie and bar inside text #3130

Merged

camelCase

eea273b

alexcjohnson reviewed Nov 1, 2018

View reviewed changes

test/jasmine/tests/violin_test.js Outdated Show resolved Hide resolved

Coding-with-Adam added 3 commits November 1, 2018 17:28

remove another commented line and a useless loop

8885b5e

remove maxHalfWidth var

57cc05a

cleaner code for vpad assignment

fe3d7ed

alexcjohnson reviewed Nov 1, 2018

View reviewed changes

etpinard added this to the v1.43.0 milestone Nov 5, 2018

etpinard added 2 commits November 6, 2018 12:07

inherit violin 'width' from box.width

44fa269

rm useless attrs in test mock

f5ba38c

etpinard added 3 commits November 6, 2018 16:59

mention that 'width' is in data coordinates

34722d3

ignore box/violin *gap and *groupgap when 'width' is set

d1aed48

do not coerce scale* attrs when violin 'width' is set

c70f7d5

- do not fill in fullLayout._violinScaleGroupStats for traces with set 'width' - use maxKDE key instead of maxWidth to avoid confusing

Merge pull request #3222 from plotly/joyplots-etpinard

0dea0e9

Joyplots etpinard

etpinard reviewed Nov 7, 2018

View reviewed changes

Coding-with-Adam added 3 commits November 7, 2018 13:06

vpadplus and minus dont cut off jitter:0 points on violin

5e5f9ee

Merge branch 'joyplots' of https://github.com/plotly/plotly.js into j…

d04c60c

…oyplots merging in Etienne's edits

fix incorrect var name

91497e5

small syntax - infix operator spacing

657848f

Kully mentioned this pull request Nov 9, 2018

Joyplots2 #3234

Merged

etpinard closed this Nov 12, 2018

etpinard deleted the joyplots branch November 16, 2018 22:17

etpinard mentioned this pull request Nov 23, 2018

882 - Chart Title Alignment #3276

Merged

etpinard removed this from the v1.43.0 milestone Dec 11, 2018

	width: {
	valType: 'number',
	dflt: null,
	min: 0,
	arrayOk: true,
	role: 'info',
	editType: 'calc',
	description: [
	'Sets the bar width (in position axis units).'
	].join(' ')
	},

add 'width' to box and violin trace #3109

add 'width' to box and violin trace #3109

Conversation

Kully commented Oct 15, 2018 • edited Loading

alexcjohnson commented Oct 16, 2018 • edited by Kully Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kully Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kully Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Kully commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kully Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

etpinard commented Nov 6, 2018

etpinard commented Nov 6, 2018

alexcjohnson commented Nov 6, 2018

Kully commented Nov 6, 2018

Kully commented Nov 7, 2018

etpinard Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kully commented Nov 7, 2018

Kully commented Nov 7, 2018 • edited Loading

etpinard commented Nov 12, 2018

Kully commented Oct 15, 2018 •

edited

Loading

alexcjohnson commented Oct 16, 2018 •

edited by Kully

Loading

Kully Oct 24, 2018 •

edited

Loading

Kully Oct 24, 2018 •

edited

Loading

Kully Nov 1, 2018 •

edited

Loading

etpinard Nov 7, 2018 •

edited

Loading

Kully commented Nov 7, 2018 •

edited

Loading