Feature: draw percentiles in violinplot #14107

sasoripathos · 2019-04-30T20:25:32Z

PR Summary

Resolve #8532

Added an optional argument, a list for percentiles, to violinplot function. User now can specify the percentiles they want to draw for each violin.

I'm opening this as part of coursework from University of Toronto Scarborough's CSCD01 taught by Dr. Anya Tafliovich @atafliovich. (https://www.utsc.utoronto.ca/~atafliovich/cscd01/index.html)

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

dstansby

Thanks a lot for submitting this pull request! Overall it looks good to me, I've just added a few comments for small things that need changing. The other thing is test images, we are trying to not add any new images to the repository if possible. I think the one image you change (not added) is a good enough test, so I think it would be worth getting rid of the new image comparison tests you have added (sorry). Thanks again for the PR.

lib/matplotlib/axes/_axes.py

lib/matplotlib/cbook/__init__.py

sasoripathos · 2019-05-01T18:00:55Z

Hi @dstansby,
I have updated documentation as your suggestions and also removed the extra image-compare test cases. Please have a check!

dstansby

👍 looks good to me now, thanks for the contribution! (we wait for two approvals to merge, so hopefully someone else will have a look at it soon)

anntzer · 2019-05-06T15:45:47Z

fwiw I would prefer this to use quantiles (https://docs.scipy.org/doc/numpy/reference/generated/numpy.quantile.html) rather than percentiles (https://docs.scipy.org/doc/numpy/reference/generated/numpy.percentile.html).

QuLogic · 2019-05-07T02:07:52Z

Please rebase to remove the extra test images entirely, or please squash merge this PR when it's accepted.

sasoripathos · 2019-05-07T02:16:09Z

@anntzer @timhoffm @QuLogic I tried to use numpy.quantile, and I got error only in python3.6 (as the failing test result in commit a3912a9) which says "numpy does not have attribute quantile". After investigation, I found numpy.quantile is newly added in numpy version 1.15.0, but currently in requirements/testing/travis36minver.txt numpy version is specified as 1.11.0.

So if we want to use numpy.quantile instead of numpy.percentile, I think the version number for numpy in requirements/testing/travis36minver.txt should be changed. Is that acceptable? Or I can revert back to the version using numpy.percentile.

This reverts commit ee5cd08.

timhoffm · 2019-05-07T06:11:49Z

I would use quantile in the API and implement it using np.percentile(100 * q).

That said, I‘m not 100% sure which of quantile/percentile is better/more common. Anybody has experience with what‘s used in communities that actually use Violinplot?

sasoripathos · 2019-05-08T01:03:54Z

I updated the code following @timhoffm's suggestion - using quantile in the API and implement it using np.percentile. Is it ok to merge?

timhoffm · 2019-05-08T20:52:37Z

lib/matplotlib/axes/_axes.py

@@ -7880,14 +7880,14 @@ def matshow(self, Z, **kwargs):
    @_preprocess_data(replace_names=["dataset"])
    def violinplot(self, dataset, positions=None, vert=True, widths=0.5,
                   showmeans=False, showextrema=True, showmedians=False,
-                   points=100, bw_method=None):
+                   percentiles=None, points=100, bw_method=None):


Name must be changed to quantiles. The implementation correctly implements quantile-behavior.

Code and documentation updated. Please have a check!

timhoffm · 2019-05-08T20:55:45Z

Not found too much on the quantile vs. percentile topic. Single source of information is ggplot, which uses quantiles. So, I think it's right for us to prefer quantiles as well.

…lib-1 into feature8532

timhoffm · 2019-05-09T21:35:16Z

lib/matplotlib/axes/_axes.py

@@ -7953,6 +7958,11 @@ def violinplot(self, dataset, positions=None, vert=True, widths=0.5,
          - ``cmedians``: A `~.collections.LineCollection` instance that
            marks the median values of each of the violin's distribution.

+          - ``cquantiles``: A
+            :class:`matplotlib.collections.LineCollection` instance created to


Suggested change

:class:`matplotlib.collections.LineCollection` instance created to

`.LineCollection` instance created to

You can shorten this since we use the sphinx option default_role = 'obj' See also
https://matplotlib.org/devdocs/devel/documenting_mpl.html?highlight=documenting#referring-to-other-code

timhoffm · 2019-05-09T21:36:27Z

lib/matplotlib/axes/_axes.py

@@ -8043,13 +8056,20 @@ def violin(self, vpstats, positions=None, vert=True, widths=0.5,

          - ``cmedians``: A `~.collections.LineCollection` instance that
            marks the median values of each of the violin's distribution.
+
+          - ``cquantiles``: A
+            :class:`matplotlib.collections.LineCollection` instance created to


timhoffm · 2019-05-09T21:39:47Z

lib/matplotlib/axes/_axes.py

@@ -7997,6 +8008,8 @@ def violin(self, vpstats, positions=None, vert=True, widths=0.5,

          - ``max``: The maximum value for this violin's dataset.

+          - ``quantiles``: The quantile values for this violin's dataset.


Should be an optional key to maintain backward compatibility with existing inputs.

timhoffm · 2019-05-09T21:42:15Z

lib/matplotlib/axes/_axes.py

@@ -8106,6 +8126,7 @@ def violin(self, vpstats, positions=None, vert=True, widths=0.5,
            mins.append(stats['min'])
            maxes.append(stats['max'])
            medians.append(stats['median'])
+            quantiles = np.concatenate((quantiles, stats['quantiles']))


You should use stats.get('quantiles') and appropriately handle the case, when it's not there to maintain backward compatibility.

Also not sure if this should be flat or 2D.

2D is not supported by the function prep_lines (typically vlines() and hlines()). In other word, these two functions requires the input at most 1D

timhoffm · 2019-05-09T21:47:56Z

lib/matplotlib/axes/_axes.py

@@ -7916,6 +7916,11 @@ def violinplot(self, dataset, positions=None, vert=True, widths=0.5,
        showmedians : bool, default = False
          If `True`, will toggle rendering of the medians.

+        quantiles : array-like, default=None


Suggested change

quantiles : array-like, default=None

quantiles : array-like, default = None

timhoffm · 2019-05-09T21:50:27Z

lib/matplotlib/cbook/__init__.py

@@ -1469,6 +1475,7 @@ def violin_stats(X, method, points=100):
        - median: The median value for this column of data.
        - min: The minimum value for this column of data.
        - max: The maximum value for this column of data.
+        - quantiles: The quantile values for this column of data.


This is a breaking API change. I'm afraid, there's no way around that, but it at least needs an API change note.

I am confused. If user directly use this function as API, my code only extends the results that user can get. If this extension is considered as a "break", then I don't think it is necessary to make qunatiles key optional. According to current code, the option, whether to add a Linecollection, is decided by violin().

If user never directly use this function, then it is only called by violinplot(). Should this be counted as a API though?

Matplotlib is extensively used out in the wild. By experience, for every remote but possible way of using the official API, there's someone out there, who actually has code with it. Therefore, we've become quite sensitive to possible API breaks. They will pop up as bug reports sooner or later. So we have to at least document the changes.

Tuple unpacking of the result will break with the change:

coords, vals, mean, median, min, max = violin_stats()

I agree to have a document. But I feel it is odd to make quantiles optional since the current code in violin_stats doesn't care whether those value will be drawn out.

I'm sorry, I misread the documentation here. It's a list of dictionaries with one dictionary per input dataset. Adding keys to these dicts is not breaking the API. So the above API change note is not necessary and should be removed again. Sorry for the noise.

timhoffm · 2019-05-09T21:58:07Z

lib/matplotlib/cbook/__init__.py

+    # Zip x and quantiles
+    data = zip(X, quantiles)
+
+    for (x, p) in data:


Can be shortend to:

Suggested change

for (x, p) in data:

for (x, q) in zip(X, quantiles):

timhoffm · 2019-05-09T21:59:02Z

lib/matplotlib/cbook/__init__.py

        # Dictionary of results for this distribution
        stats = {}

        # Calculate basic stats for the distribution
        min_val = np.min(x)
        max_val = np.max(x)
+        p_val = np.percentile(x, 100 * p)


Suggested change

p_val = np.percentile(x, 100 * p)

quantiles_val = np.percentile(x, 100 * q)

timhoffm · 2019-05-09T22:22:00Z

lib/matplotlib/tests/test_axes.py

+
+    # Different size quantile list and plots
+    with pytest.raises(ValueError):
+        ax.violinplot(data, quantiles=[[0.1, 0.2], [0.5]])


Actually, this API doesn't quite make sense. You allow quantiles=[[0.1, 0.9], [0.2, 0.8]] but not quantiles=[[0.1, 0.2], [0.5]].

Either there is just a simple case that has the same quantiles for every postion. Or one can freely define the quantiles per position. Just allowing the same number of quantiles but with different values is an odd cut.

That said, I'm unclear if there is a practical need for different quantiles per position. It prevents comparability of the quantile lines between positions.

I don't think it is odd. I feel it perfectly makes sense to allow user draw different quantiles for different violins. I think in this implementation user still can draw same quantiles for each violin easily. But if only support same quantiles for every postion, it would be hard if users really want to draw different quantiles for different violins.

But if the quantiles can be different, why should you prohibit different numbers of quantiles between violins? Either values and number of quantiles must be the same for all, or values and number can vary.

No, this example is user wants to plot only one violin, but gives two lists of quantiles. The ValueError is cause by number of violins != number of quantile lists, ie. 1 != 2

If you look at the updated code in example folder, my design accepts two violins have different number and value of quantiles. (ie. [0.1, 0.2] for first violin, [0.5, 0.7, 0.9] for second violin)

Sorry, I misinterpreted the test. Can you please use the same number of quantiles for both lists. Makes it hopefully more obvious that the number of elements in the sublists is not the issue to be tested.

Sure, I will update it with some other changes you mentioned/suggested.

Add next API change document for violin_stats in cbook. Make violin() more backward campatible by making quantiles key optional. Update smoke test for violin number and quantile lists number mismatch.

sasoripathos · 2019-05-10T02:23:07Z

@timhoffm I updated the docstring, code and test cases based on your suggestions. I also added a .rst doc in next_api_change folder. Please have a check.

timhoffm

Looks good. Please remove the API change again and it will be ready to go. Thanks for your effort!

timhoffm · 2019-05-11T17:10:00Z

lib/matplotlib/cbook/__init__.py

@@ -1469,6 +1475,7 @@ def violin_stats(X, method, points=100):
        - median: The median value for this column of data.
        - min: The minimum value for this column of data.
        - max: The maximum value for this column of data.
+        - quantiles: The quantile values for this column of data.


I'm sorry, I misread the documentation here. It's a list of dictionaries with one dictionary per input dataset. Adding keys to these dicts is not breaking the API. So the above API change note is not necessary and should be removed again. Sorry for the noise.

Comments handled.

sasoripathos · 2019-05-11T20:04:11Z

@timhoffm API change doc removed.

timhoffm · 2019-05-11T21:07:58Z

Thanks! And congratulations on your first contribution to matplotlib!

sasoripathos added 15 commits March 22, 2019 12:40

Fix issue 10788

e1985d7

Fix style

2279b16

Add missing import statement

19bb0c7

Merge remote-tracking branch 'upstream/master'

b7fdc9b

Merge remote-tracking branch 'upstream/master'

109f252

Merge remote-tracking branch 'upstream/master'

b2ca609

Merge remote-tracking branch 'upstream/master'

2c5a873

Remove changes

cb0a64c

Code for feature 8532

67664d5

Run boilerplate

ea5dd1c

Fix code style

5552d3d

Remove unused test images

3fd9152

Update pyplot.py

80d3fd8

Update pyplot.py

f78fcac

Run boilerplate

dbda148

dstansby added this to the v3.2.0 milestone May 1, 2019

dstansby requested changes May 1, 2019

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

lib/matplotlib/cbook/__init__.py Outdated Show resolved Hide resolved

lib/matplotlib/cbook/__init__.py Outdated Show resolved Hide resolved

sasoripathos added 3 commits May 1, 2019 12:42

Remove extra image-compare tests, update documentation

fb32794

Merge remote-tracking branch 'upstream/master'

b8ae7bd

Merge branch 'master' into feature8532

0bb79ec

dstansby approved these changes May 6, 2019

View reviewed changes

sasoripathos added 5 commits May 6, 2019 20:59

Merge remote-tracking branch 'upstream/master' into feature8532

3d46d7f

Use np.quantile instead of percentile

a3912a9

Merge remote-tracking branch 'upstream/master'

6e50219

Update test images for violinplot

ee5cd08

Merge branch 'master' into feature8532

999a424

Revert "Update test images for violinplot"

eafd766

This reverts commit ee5cd08.

sasoripathos added 3 commits May 7, 2019 19:48

Use np.percentile implement quantile

e48658f

Merge remote-tracking branch 'upstream/master'

8cbafae

Merge branch 'master' into feature8532

6f3e2a0

timhoffm previously requested changes May 8, 2019

View reviewed changes

sasoripathos added 4 commits May 8, 2019 20:36

Update api from percentiles to quantiles

7a4777b

Merge branch 'feature8532' of https://github.com/sasoripathos/matplot…

5d80c50

…lib-1 into feature8532

Update violinplot call in test cases

6afe73a

Update documentations from percentiles to quantiles

54f955a

timhoffm reviewed May 9, 2019

View reviewed changes

Update docstring and backward campatibility

a0c3073

Add next API change document for violin_stats in cbook. Make violin() more backward campatible by making quantiles key optional. Update smoke test for violin number and quantile lists number mismatch.

timhoffm reviewed May 11, 2019

View reviewed changes

Remove unnecessary api change doc

c586932

timhoffm approved these changes May 11, 2019

View reviewed changes

timhoffm merged commit b6d3f7c into matplotlib:master May 11, 2019

sasoripathos deleted the feature8532 branch May 11, 2019 22:17

timhoffm mentioned this pull request May 12, 2019

Added support for displaying percentiles to violinplot function #9971

Closed

6 tasks

timhoffm mentioned this pull request May 19, 2019

Add decorator to inherit keyword-only deprecations #14130

Closed

2 tasks

story645 mentioned this pull request Nov 1, 2019

add explanations and examples to the docstring of violinplot #12585

Closed

	:class:`matplotlib.collections.LineCollection` instance created to
	`.LineCollection` instance created to

		@@ -7997,6 +8008,8 @@ def violin(self, vpstats, positions=None, vert=True, widths=0.5,

		- ``max``: The maximum value for this violin's dataset.

		- ``quantiles``: The quantile values for this violin's dataset.

	quantiles : array-like, default=None
	quantiles : array-like, default = None

	p_val = np.percentile(x, 100 * p)
	quantiles_val = np.percentile(x, 100 * q)

Feature: draw percentiles in violinplot #14107

Feature: draw percentiles in violinplot #14107

Conversation

sasoripathos commented Apr 30, 2019 • edited Loading

PR Summary

PR Checklist

dstansby left a comment

Choose a reason for hiding this comment

sasoripathos commented May 1, 2019

dstansby left a comment

Choose a reason for hiding this comment

anntzer commented May 6, 2019

QuLogic commented May 7, 2019

sasoripathos commented May 7, 2019 • edited Loading

timhoffm commented May 7, 2019 • edited Loading

sasoripathos commented May 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm commented May 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasoripathos May 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm May 10, 2019 • edited Loading

Choose a reason for hiding this comment

sasoripathos May 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm May 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasoripathos commented May 10, 2019

timhoffm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasoripathos commented May 11, 2019

timhoffm commented May 11, 2019

sasoripathos commented Apr 30, 2019 •

edited

Loading

sasoripathos commented May 7, 2019 •

edited

Loading

timhoffm commented May 7, 2019 •

edited

Loading

sasoripathos commented May 8, 2019 •

edited

Loading

sasoripathos May 10, 2019 •

edited

Loading

timhoffm May 10, 2019 •

edited

Loading

sasoripathos May 10, 2019 •

edited

Loading

timhoffm May 10, 2019 •

edited

Loading