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

Add Violin Element #2114

Merged
merged 18 commits into from Feb 5, 2018

Conversation

@philippjfr
Copy link
Member

philippjfr commented Nov 11, 2017

Since we now have a univariate_kde operation implementing a Violin plot was pretty easy.

Here's what the element can do, it acts much like BoxWhisker: https://anaconda.org/philippjfr/violin/notebook

bokeh_plot

image

  • Added matplotlib plotting tests
  • Added bokeh plotting tests
  • Added Element reference notebooks
  • Added gallery examples
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 12, 2017

Very happy to see this element added for the next release.

Other than the obvious merge conflict that needs resolving, I just want to add a reminder about that streams + compositor issue (as these stats elements use the compositor afaik). Just something extra to make sure is working as the compositor will need a bit more work to handle streams properly.

@philippjfr

This comment has been minimized.

Copy link
Member Author

philippjfr commented Dec 13, 2017

This particular element does not use compositors. I think this is mostly ready to merge (after resolving merge conflicts). I have improvements to coloring the violins in mind but they will require some work in bokeh to allow multi_line and patches glyphs on categorical axes again.

@philippjfr philippjfr force-pushed the violin branch 2 times, most recently from cd05836 to 8232ef8 Jan 28, 2018
@philippjfr philippjfr force-pushed the violin branch from 585f4ea to c5c1647 Feb 3, 2018
@philippjfr

This comment has been minimized.

Copy link
Member Author

philippjfr commented Feb 3, 2018

Ready to review and merge.

philippjfr added 2 commits Feb 3, 2018
median, mean and various percentiles. It may have a single value
dimension and any number of key dimensions declaring the grouping
of each violin.
"""

This comment has been minimized.

Copy link
@jlstevens

jlstevens Feb 5, 2018

Contributor

I assume there a reason BoxWhisker can't also go into element/stats.py?

This comment has been minimized.

Copy link
@philippjfr

philippjfr Feb 5, 2018

Author Member

Yes, I initially moved it there, but it breaks unpickling of our test data and I can't move it and import it into charts because that would be circular.

This comment has been minimized.

Copy link
@jlstevens

jlstevens Feb 5, 2018

Contributor

Ok. Should this be marked as something to fix in version 2.0 when we can break backwards compatibility?

This comment has been minimized.

Copy link
@philippjfr

philippjfr Feb 5, 2018

Author Member

Yes, probably.

This comment has been minimized.

Copy link
@philippjfr

philippjfr Feb 5, 2018

Author Member

Should I just file an issue once this is merged?

@@ -16,7 +16,7 @@ def _kde_support(bin_range, bw, gridsize, cut, clip):
if clip[0] is not None and np.isfinite(clip[0]):
kmin = max(kmin, clip[0])
if clip[1] is not None and np.isfinite(clip[1]):
kmax = max(kmax, clip[1])
kmax = min(kmax, clip[1])

This comment has been minimized.

Copy link
@jlstevens

jlstevens Feb 5, 2018

Contributor

Given this function existed before this PR, what might have been affected by this bug? (if anything?)

This comment has been minimized.

Copy link
@philippjfr

philippjfr Feb 5, 2018

Author Member

Distribution elements would have been clipped incorrectly but I think clipping is disabled by default so not all that much would have changed.


cut = param.Number(default=5, doc="""
Draw the estimate to cut * bw from the extreme data points.""")

This comment has been minimized.

Copy link
@jlstevens

jlstevens Feb 5, 2018

Contributor

Looks like the matplotlib implementation doesn't have the clip and cut plot options?

This comment has been minimized.

Copy link
@philippjfr

philippjfr Feb 5, 2018

Author Member

Right, the matplotlib violin implementation doesn't expose it for some reason, it just always clips.

This comment has been minimized.

Copy link
@jlstevens

jlstevens Feb 5, 2018

Contributor

Ok. That is fine as long as these defaults match the behavior of the matplotlib version.

This comment has been minimized.

Copy link
@philippjfr

philippjfr Feb 5, 2018

Author Member

It does.

@philippjfr

This comment has been minimized.

Copy link
Member Author

philippjfr commented Feb 5, 2018

I think this is ready to merge.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 5, 2018

You replied to all my queries, thanks!

Looks good and tests are passing. Merging.

@jlstevens jlstevens merged commit c83b96e into master Feb 5, 2018
4 checks passed
4 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 81.061%
Details
s3-reference-data-cache Tests passing no test data changes required.
Details
@philippjfr philippjfr deleted the violin branch Feb 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.