Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
add 'width' to box and violin trace #3109
Changes from 27 commits
8f04d13
f76ca97
81cdbe9
5ef2eeb
1c6b26b
2d93a0e
1f9ced8
26f44c4
77b9c9d
243a369
eb5e9da
eea273b
408aacb
0555283
4eb8b44
44e0a88
f8b963b
54aa355
0d4239b
fa3a314
c63780f
c24330d
6e7883d
37786d6
45ae47d
8366c88
ce81377
8885b5e
57cc05a
fe3d7ed
44fa269
f5ba38c
34722d3
d1aed48
c70f7d5
0dea0e9
5e5f9ee
d04c60c
91497e5
657848f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌴
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed another bug - if we choose to show the box, it gets cut in half if we choose

positive
ornegative
forside
DATA:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sorry, DRY - I guess that's just a plotly.js convention.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anything break if we 🔪 the
else
entirely? Unnecessary attributes should simply not be included intraceOut
at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the violins are not drawn properly: the outlines are filled with black ink
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kully do you know why this test had to be modified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't too sure to be honest. I ran the test a few times but there were only two values the assert was saying was valid, iirc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably it changed because autorange can shrink depending on
side
now, so the violin moved.