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.

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

Closed
wants to merge 40 commits into from
Closed

add 'width' to box and violin trace #3109

Show file tree
Hide file tree
Changes from 2 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
3 changes: 3 additions & 0 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2612,6 +2612,9 @@ function doCrossTraceCalc(gd) {
fullLayout[sp];

for(j = 0; j < methods.length; j++) {
console.log(methods);
console.log(methods[j])
console.log(methods[j].type)
methods[j](gd, spInfo, sp);
}
}
Expand Down
13 changes: 12 additions & 1 deletion src/traces/box/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,5 +255,16 @@ 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 ,

vwidth: {
valType: 'number',
min: 0,
role: 'info',
dflt: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 😁

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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']?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

editType: 'calc',
description: [
'Sets the width of the violins.',
'This overrides the normal width of the violins.'
].join(' ')
},
};
9 changes: 7 additions & 2 deletions src/traces/box/cross_trace_calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ function crossTraceCalc(gd, plotinfo) {
}
}

setPositionOffset('box', gd, boxList, posAxis, [minPad, maxPad]);
setPositionOffset('box', gd, boxList, posAxis, [minPad, maxPad], false);
}
}

function setPositionOffset(traceType, gd, boxList, posAxis, pad) {
function setPositionOffset(traceType, gd, boxList, posAxis, pad, vwidth) {
var calcdata = gd.calcdata;
var fullLayout = gd._fullLayout;
var pointList = [];
Expand All @@ -77,6 +77,11 @@ function setPositionOffset(traceType, gd, boxList, posAxis, pad) {
var boxdv = Lib.distinctVals(pointList);
var dPos = boxdv.minDiff / 2;

// override dPos if violin width given
if (vwidth != false) {
dPos = vwidth;
}

// if there's no duplication of x points,
// disable 'group' mode by setting counter to 1
if(pointList.length === boxdv.vals.length) {
Expand Down
1 change: 1 addition & 0 deletions src/traces/box/defaults.js
View file
Open in desktop
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('vwidth');

var notched = coerce('notched', traceIn.notchwidth !== undefined);
if(notched) coerce('notchwidth');
Expand Down
11 changes: 11 additions & 0 deletions src/traces/violin/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@ var boxAttrs = require('../box/attributes');
var extendFlat = require('../../lib/extend').extendFlat;

module.exports = {
vwidth: {
valType: 'number',
min: 0,
role: 'info',
dflt: false,
editType: 'calc',
description: [
'Sets the width of the violins.',
'This overrides the normal width of the violins.'
].join(' ')
},
y: boxAttrs.y,
x: boxAttrs.x,
x0: boxAttrs.x0,
Expand Down
2 changes: 1 addition & 1 deletion src/traces/violin/cross_trace_calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,6 @@ module.exports = function crossTraceCalc(gd, plotinfo) {
}
}

setPositionOffset('violin', gd, violinList, posAxis, [minPad, maxPad]);
setPositionOffset('violin', gd, violinList, posAxis, [minPad, maxPad], trace.vwidth);
}
};
1 change: 1 addition & 0 deletions src/traces/violin/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('scalegroup', traceOut.name);
coerce('scalemode');
coerce('side');
coerce('vwidth');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


var span = coerce('span');
var spanmodeDflt;
Expand Down