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 1 commit
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
26 changes: 19 additions & 7 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 @@ -90,23 +90,35 @@ 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.vwidth) {
if(calcTrace[0].trace.vwidth / 2 > max_half_width) {
max_half_width = calcTrace[0].trace.vwidth / 2;
}
}
}

// 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 •
edited
Loading

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 •
edited
Loading

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 and
// override this width with
// trace.width if it exists
// set the width of all boxes
// override dPos with trace.width if present
if(calcTrace[0].trace && calcTrace[0].trace.vwidth) {
calcTrace[0].t.dPos = calcTrace[0].trace.vwidth;
calcTrace[0].t.dPos = calcTrace[0].trace.vwidth / 2;
} else {
calcTrace[0].t.dPos = dPos;
calcTrace[0].t.dPos = dPos;
}
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


// link extremes to all boxes
Expand Down
9 changes: 2 additions & 7 deletions src/traces/box/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,8 @@ function supplyDefaults(traceIn, traceOut, defaultColor, layout) {

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

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

var notched = coerce('notched', traceIn.notchwidth !== undefined);
if(notched) coerce('notchwidth');
Expand Down
9 changes: 3 additions & 6 deletions src/traces/violin/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,9 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout

coerce('bandwidth');
coerce('side');

var vwidth = coerce('vwidth');
if(!vwidth) {
coerce('scalegroup', traceOut.name);
coerce('scalemode');
}
coerce('scalegroup', traceOut.name);
coerce('scalemode');
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