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

Joyplots2 #3234

Merged
merged 62 commits into from
Jan 21, 2019
Merged

Joyplots2 #3234

merged 62 commits into from
Jan 21, 2019

Conversation

Kully
Copy link
Contributor

@Kully Kully commented Nov 9, 2018

Rebasing from this commit from original Joyplots PR #3109

@etpinard
Copy link
Contributor

etpinard commented Jan 8, 2019

@plotly/plotly_js tagging this as reviewable. It's about time!

The improved algo is in bf6cfa2 which 🔪 all our concerns from #3234 (comment) (I think 😉 ).

Jasmine tests are now passing thanks to 12ff323 🎉

@etpinard
Copy link
Contributor

@Kully could you take a few minutes this week to double-check my latest attempt?

"y0": 0.2
}],
"layout": {
"title": "Joyplot - Violin with multiple widths",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get rid of the name "joyplot" here if possible, in favour of "ridgeplot"

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok done in c1d6e76

@etpinard
Copy link
Contributor

@antoinerg this PR is a bit of a mess, but could I ask you to review it before next Tuesday (the 22nd)?

... and thus allow single-box groups!
@antoinerg
Copy link
Contributor

antoinerg commented Jan 21, 2019

@etpinard Looks very nice!

However, I noticed that the axis range does not always update properly. For example, for the mock violin_box_multiple_widths.json, when I hide the box with width=0.6 by clicking on its name on the legend, I end up with:
violin_box_multiple_widths_unselect_box
As you can see, one of the violin is clipped. Is this the expected behavior?

@etpinard
Copy link
Contributor

when I hide the box by clicking on its name on the legend I end up with:

Thanks. Fixed in a237252

@antoinerg
Copy link
Contributor

antoinerg commented Jan 21, 2019

Nice fix 👌 !

Another question, this one is about the current behavior for automatic width. We resize the violins even when the axis range isn't modified: for example, the auto trace goes from small to very large upon hiding trace witdh: 0.4:
screenshot_2019-01-21_15-02-57_01
screenshot_2019-01-21_15-03-21_02

I just want to make sure this is what we want. Maybe we should add a Jasmine test to 🔒 that down?

@etpinard let me know what you think!

@etpinard
Copy link
Contributor

Maybe we should add a Jasmine test to that down?

That change in violin width comes from

var boxdv = Lib.distinctVals(pointList);
var dPos0 = boxdv.minDiff / 2;

where we have two distinct violin position vals at first, and then just once after that restyle call.

Here's the new test: ef52b8c

@antoinerg
Copy link
Contributor

Great job @etpinard ! 💃

@etpinard
Copy link
Contributor

Thanks for catching the bug from #3234 (comment) @antoinerg !

@etpinard
Copy link
Contributor

In brief, this PR:

  • adds width to box and violin traces
  • improves box/violin autorange calculations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants