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

Implement notched box plots, closes #2286 #2305

Merged
merged 7 commits into from Jan 30, 2018

Conversation

Projects
None yet
3 participants
@krassowski
Copy link
Contributor

commented Jan 29, 2018

I added a boolean option "notched" (default false) and a numerical "notchwidth" (default 0.25).
The options define if the box trace should be notched or not and if yes, determine the width of the notch (proportional to the width of the box).

I modified the svg path-generation code, utilizing conditional (ternary) operators so the common parts of the box (top & bottom edges for vertical, left & right edges for horizontal) are shared.

I added two test baselines: box_notched and box_horz_notched.

Preliminary review was given by @alexcjohnson in my fork: krassowski#1.

Update: notch width behavior was changed to follow Matlab convention.
The previous version of this PR used the ggplot2 way.

Please let me know what you think.

box notched

box horiz notched

@@ -29,6 +29,9 @@ function supplyDefaults(traceIn, traceOut, defaultColor, layout) {
coerce('whiskerwidth');
coerce('boxmean');

var notched = coerce('notched');
if(notched) coerce('notchwidth');

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 29, 2018

Member

I wonder if we can get away with just a notchwidth similar to how whiskers are set (with whiskerwidth)?

This comment has been minimized.

Copy link
@krassowski

krassowski Jan 29, 2018

Author Contributor

Are you suggesting to remove if(notched)?
Btw. this line was modified after this comment: krassowski#1 (comment)

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 29, 2018

Member

Are you suggesting to remove if(notched)?

Yes, that's what I'm suggesting. Waiting on a concensus.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Jan 29, 2018

Contributor

I think @etpinard was suggesting removing the notched attribute altogether and just defaulting to notchwidth=0 as no notch. I thought about this but would prefer the existing way so that we can pick a nice default notchwidth rather than everyone making up random values. One thing we could do though is make the default for notched be true if traceIn.notchwidth !== undefined. That way if you do want a custom width you don’t also need to provide notched.

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 30, 2018

Member

was suggesting removing the notched attribute altogether and just defaulting to notchwidth=0 as no notch

yes, this is what I meant. Sorry for the confusion.

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 30, 2018

Member

I thought about this but would prefer the existing way so that we can pick a nice default notchwidth rather than everyone making up random values

Good point here. I wonder if we should do the same for whiskers in v2? That is, adding a showwhiskers boolean and making the whiskerswidth default smarter?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Jan 30, 2018

Contributor

I wonder if we should do the same for whiskers in v2?

My argument doesn't really apply to whiskers - though I'm not sure I get what you mean by smarter default. If we're happy with the default whiskerwidth staying at 0.5 then people can use 0 (a very non-random number 😄 ) to disable whiskers and any other number for a custom width.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Jan 30, 2018

Contributor

@krassowski in talking with @etpinard offline, we agreed that despite the correspondence with whiskerwidth the Matlab convention makes more sense, to us anyway - so that's 0=no notch, 0.5=meet in the middle, with a default of 0.25.

And re: my comment:

One thing we could do though is make the default for notched be true if traceIn.notchwidth !== undefined.

That's just removing attributes.notched.dflt and changing its coerce line to:
var notched = coerce('notched', traceIn.notchwidth !== undefined);

You'll also want to merge in the latest master so tests pass, then I think we're ready to go!

krassowski added some commits Jan 30, 2018

Implement 'smarter' notched/notchwidth default
I deliberately removed `"notched":true` in one of test
box traces so it is (indirectly) tested
@@ -101,6 +101,8 @@ function plotBoxAndWhiskers(sel, axes, trace, t) {
var wdPos = t.wdPos || 0;
var bPosPxOffset = t.bPosPxOffset || 0;
var whiskerWidth = trace.whiskerwidth || 0;
var notched = trace.notched || false;
var nw = notched ? 1 - trace.notchwidth : 1;

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Jan 30, 2018

Contributor

I think we're off by a factor of 2 here - 1 - 2 * trace.notchwidth?

This comment has been minimized.

Copy link
@krassowski

krassowski Jan 30, 2018

Author Contributor

So notchedwidth will accept a float from range (0, 0.5) instead of current (0, 1)?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Jan 30, 2018

Contributor

haha simultaneous comments :) yes.

notchwidth: {
valType: 'number',
min: 0,
max: 1,

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Jan 30, 2018

Contributor

also in the matlab convention the max is 0.5

Set notch width range to (0, 0.5) instead of (0,1)
presumably for better consistency with Matlab
@krassowski

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

It seems that test "select box and lasso per trace" fails. I have no idea why.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

It seems that test "select box and lasso per trace" fails. I have no idea why.

Seems to be a bad day for that test. You didn't break it, it's just glitchy.

💃 - once tests pass I'll merge this. Thanks for your work @krassowski!

"name":"Notch width = 0.1",
"notched":true,
"orientation": "h",
"notchwidth":0.1

This comment has been minimized.

Copy link
@krassowski

krassowski Jan 30, 2018

Author Contributor

You are right. I updated the baselines, hope that all will work this time.

@alexcjohnson alexcjohnson added this to the v1.34.0 milestone Jan 30, 2018

@alexcjohnson alexcjohnson merged commit 28f7ea2 into plotly:master Jan 30, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.