Skip to content

Fix box & violin inner parts removal #2785

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 5 commits into from
Jul 6, 2018
Merged

Fix box & violin inner parts removal #2785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
611581c
sub fail -> failTest
etpinard Jul 5, 2018
47349c7
fix box line/pts removal
etpinard Jul 5, 2018
4e48cd7
fix violin box/meanline/pts removal
etpinard Jul 5, 2018
b2a55f6
fixup new box.plot logic for candlestick
etpinard Jul 5, 2018
1e4900c
cleanup logic for data-bind fn / fallback to []
etpinard Jul 6, 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
51 changes: 29 additions & 22 deletions src/traces/box/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,9 @@ function plot(gd, plotinfo, cdbox, boxLayer) {
// always split the distance to the closest box
t.wHover = t.dPos * (group ? groupFraction / numBoxes : 1);

// boxes and whiskers
plotBoxAndWhiskers(sel, {pos: posAxis, val: valAxis}, trace, t);

// draw points, if desired
if(trace.boxpoints) {
plotPoints(sel, {x: xa, y: ya}, trace, t);
}

// draw mean (and stdev diamond) if desired
if(trace.boxmean) {
plotBoxMean(sel, {pos: posAxis, val: valAxis}, trace, t);
}
plotPoints(sel, {x: xa, y: ya}, trace, t);
plotBoxMean(sel, {pos: posAxis, val: valAxis}, trace, t);
});
}

Expand All @@ -109,7 +100,14 @@ function plotBoxAndWhiskers(sel, axes, trace, t) {
bdPos1 = t.bdPos;
}

var paths = sel.selectAll('path.box').data(Lib.identity);
var fn;
if(trace.type === 'box' ||
(trace.type === 'violin' && (trace.box || {}).visible)
) {
fn = Lib.identity;
}

var paths = sel.selectAll('path.box').data(fn || []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this pattern! But two comments about it:

  • trace.box and trace.meanline are deleted rather than left with visible: false - so can't we just say (trace.type === 'violin' && trace.box) etc?
  • I'm a bit uneasy relying on uninitialized vars being undefined - seems like it makes things more fragile, for example if we use fn again, or wrap this block in a loop or something... I'd rather a pattern like
var fn;
if(...) fn = Lib.identity;
else fn = []; // or just var fn = []; and drop the else, dunno which is better

var paths = sel.selectAll('path.box').data(fn);


paths.enter().append('path')
.style('vector-effect', 'non-scaling-stroke')
Expand Down Expand Up @@ -187,16 +185,18 @@ function plotPoints(sel, axes, trace, t) {
// repeatable pseudo-random number generator
Lib.seedPseudoRandom();

var gPoints = sel.selectAll('g.points')
// since box plot points get an extra level of nesting, each
// box needs the trace styling info
.data(function(d) {
d.forEach(function(v) {
v.t = t;
v.trace = trace;
});
return d;
// since box plot points get an extra level of nesting, each
// box needs the trace styling info
var fn = function(d) {
d.forEach(function(v) {
v.t = t;
v.trace = trace;
});
return d;
};

var gPoints = sel.selectAll('g.points')
.data(mode ? fn : []);

gPoints.enter().append('g')
.attr('class', 'points');
Expand Down Expand Up @@ -306,7 +306,14 @@ function plotBoxMean(sel, axes, trace, t) {
bdPos1 = t.bdPos;
}

var paths = sel.selectAll('path.mean').data(Lib.identity);
var fn;
if(trace.type === 'box' && trace.boxmean ||
(trace.type === 'violin' && (trace.box || {}).visible && (trace.meanline || {}).visible)
) {
fn = Lib.identity;
}

var paths = sel.selectAll('path.mean').data(fn || []);

paths.enter().append('path')
.attr('class', 'mean')
Expand Down
111 changes: 52 additions & 59 deletions src/traces/violin/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ module.exports = function plot(gd, plotinfo, cd, violinLayer) {
var hasBothSides = trace.side === 'both';
var hasPositiveSide = hasBothSides || trace.side === 'positive';
var hasNegativeSide = hasBothSides || trace.side === 'negative';
var hasBox = trace.box && trace.box.visible;
var hasMeanLine = trace.meanline && trace.meanline.visible;
var groupStats = fullLayout._violinScaleGroupStats[trace.scalegroup];

var violins = sel.selectAll('path.violin').data(Lib.identity);
Expand Down Expand Up @@ -149,66 +147,61 @@ module.exports = function plot(gd, plotinfo, cd, violinLayer) {
d.pathLength = d.path.getTotalLength() / (hasBothSides ? 2 : 1);
});

if(hasBox) {
var boxWidth = trace.box.width;
var boxLineWidth = trace.box.line.width;
var bdPosScaled;
var bPosPxOffset;
var boxAttrs = trace.box || {};
var boxWidth = boxAttrs.width;
var boxLineWidth = (boxAttrs.line || {}).width;
var bdPosScaled;
var bPosPxOffset;

if(hasBothSides) {
bdPosScaled = bdPos * boxWidth;
bPosPxOffset = 0;
} else if(hasPositiveSide) {
bdPosScaled = [0, bdPos * boxWidth / 2];
bPosPxOffset = -boxLineWidth;
} else {
bdPosScaled = [bdPos * boxWidth / 2, 0];
bPosPxOffset = boxLineWidth;
}

if(hasBothSides) {
bdPosScaled = bdPos * boxWidth;
bPosPxOffset = 0;
} else if(hasPositiveSide) {
bdPosScaled = [0, bdPos * boxWidth / 2];
bPosPxOffset = -boxLineWidth;
} else {
bdPosScaled = [bdPos * boxWidth / 2, 0];
bPosPxOffset = boxLineWidth;
}
// inner box
boxPlot.plotBoxAndWhiskers(sel, {pos: posAxis, val: valAxis}, trace, {
bPos: bPos,
bdPos: bdPosScaled,
bPosPxOffset: bPosPxOffset
});

boxPlot.plotBoxAndWhiskers(sel, {pos: posAxis, val: valAxis}, trace, {
bPos: bPos,
bdPos: bdPosScaled,
bPosPxOffset: bPosPxOffset
});

// if both box and meanline are visible, show mean line inside box
if(hasMeanLine) {
boxPlot.plotBoxMean(sel, {pos: posAxis, val: valAxis}, trace, {
bPos: bPos,
bdPos: bdPosScaled,
bPosPxOffset: bPosPxOffset
});
}
}
else {
if(hasMeanLine) {
var meanPaths = sel.selectAll('path.mean').data(Lib.identity);

meanPaths.enter().append('path')
.attr('class', 'mean')
.style({
fill: 'none',
'vector-effect': 'non-scaling-stroke'
});

meanPaths.exit().remove();

meanPaths.each(function(d) {
var v = valAxis.c2p(d.mean, true);
var p = helpers.getPositionOnKdePath(d, trace, v);

d3.select(this).attr('d',
trace.orientation === 'h' ?
'M' + v + ',' + p[0] + 'V' + p[1] :
'M' + p[0] + ',' + v + 'H' + p[1]
);
});
}
}
// meanline insider box
boxPlot.plotBoxMean(sel, {pos: posAxis, val: valAxis}, trace, {
bPos: bPos,
bdPos: bdPosScaled,
bPosPxOffset: bPosPxOffset
});

if(trace.points) {
boxPlot.plotPoints(sel, {x: xa, y: ya}, trace, t);
var fn;
if(!(trace.box || {}).visible && (trace.meanline || {}).visible) {
fn = Lib.identity;
}

// N.B. use different class name than boxPlot.plotBoxMean,
// to avoid selectAll conflict
var meanPaths = sel.selectAll('path.meanline').data(fn || []);
meanPaths.enter().append('path')
.attr('class', 'meanline')
.style('fill', 'none')
.style('vector-effect', 'non-scaling-stroke');
meanPaths.exit().remove();
meanPaths.each(function(d) {
var v = valAxis.c2p(d.mean, true);
var p = helpers.getPositionOnKdePath(d, trace, v);

d3.select(this).attr('d',
trace.orientation === 'h' ?
'M' + v + ',' + p[0] + 'V' + p[1] :
'M' + p[0] + ',' + v + 'H' + p[1]
);
});

boxPlot.plotPoints(sel, {x: xa, y: ya}, trace, t);
});
};
14 changes: 10 additions & 4 deletions src/traces/violin/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,17 @@ module.exports = function style(gd, cd) {
.call(Color.stroke, boxLine.color)
.call(Color.fill, box.fillcolor);

var meanLineStyle = {
'stroke-width': meanLineWidth + 'px',
'stroke-dasharray': (2 * meanLineWidth) + 'px,' + meanLineWidth + 'px'
};

sel.selectAll('path.mean')
.style({
'stroke-width': meanLineWidth + 'px',
'stroke-dasharray': (2 * meanLineWidth) + 'px,' + meanLineWidth + 'px'
})
.style(meanLineStyle)
.call(Color.stroke, meanline.color);

sel.selectAll('path.meanline')
.style(meanLineStyle)
.call(Color.stroke, meanline.color);

stylePoints(sel, trace, gd);
Expand Down
58 changes: 58 additions & 0 deletions test/jasmine/tests/violin_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,3 +493,61 @@ describe('Test violin hover:', function() {
});
});
});

describe('Test violin restyle:', function() {
var gd;

beforeEach(function() {
gd = createGraphDiv();
});

afterEach(destroyGraphDiv);

it('should be able to add/remove innner parts', function(done) {
var fig = Lib.extendDeep({}, require('@mocks/violin_old-faithful.json'));
// start with just 1 violin
delete fig.data[0].meanline;
delete fig.data[0].points;

function _assertOne(msg, exp, trace3, k, query) {
expect(trace3.selectAll(query).size())
.toBe(exp[k] || 0, k + ' - ' + msg);
}

function _assert(msg, exp) {
var trace3 = d3.select(gd).select('.violinlayer > .trace');
_assertOne(msg, exp, trace3, 'violinCnt', 'path.violin');
_assertOne(msg, exp, trace3, 'boxCnt', 'path.box');
_assertOne(msg, exp, trace3, 'meanlineInBoxCnt', 'path.mean');
_assertOne(msg, exp, trace3, 'meanlineOutOfBoxCnt', 'path.meanline');
_assertOne(msg, exp, trace3, 'ptsCnt', 'path.point');
}

Plotly.plot(gd, fig)
.then(function() {
_assert('base', {violinCnt: 1});
})
.then(function() { return Plotly.restyle(gd, 'box.visible', true); })
.then(function() {
_assert('with inner box', {violinCnt: 1, boxCnt: 1});
})
.then(function() { return Plotly.restyle(gd, 'meanline.visible', true); })
.then(function() {
_assert('with inner box & meanline', {violinCnt: 1, boxCnt: 1, meanlineInBoxCnt: 1});
})
.then(function() { return Plotly.restyle(gd, 'box.visible', false); })
.then(function() {
_assert('with meanline', {violinCnt: 1, meanlineOutOfBoxCnt: 1});
})
.then(function() { return Plotly.restyle(gd, 'points', 'all'); })
.then(function() {
_assert('with meanline & pts', {violinCnt: 1, meanlineOutOfBoxCnt: 1, ptsCnt: 272});
})
.then(function() { return Plotly.restyle(gd, 'meanline.visible', false); })
.then(function() {
_assert('with pts', {violinCnt: 1, ptsCnt: 272});
})
.catch(failTest)
.then(done);
});
});