Draft version of MEP28: Simplification of boxplots by phobson · Pull Request #7282 · matplotlib/matplotlib

phobson · 2016-10-16T15:45:24Z

This a MEP for a series of small changes that will

Remove much of the the complexity (implementation, and API) from cbook.boxplot_stats, Axes.boxplot, and Axes.bxp
Provide complete control over how the statistics are computed to outside libraries like seaborn with no action required on their part
Remove some old parameter in a backwards incompatible fashion for a more consistent API.

NelleV · 2016-10-16T15:50:01Z

doc/devel/MEP/MEP28.rst

+importantly, at the very least, seaborn would require no changes to its
+API to allow users to take advantage of these new options.
+
+Simiplifications to the ``Axes.boxplot`` API and other functions


small typo : Simiplifications -> simplifications

NelleV

Here is a couple of very small typo fixes.
I'll do a second review once I have enough caffeine flowing…

NelleV · 2016-10-16T15:51:59Z

doc/devel/MEP/MEP28.rst

+   :local:
+
+
+This MEP template is a guideline of the sections that a MEP should


I think you are meant to remove this :)

NelleV · 2016-10-16T15:54:46Z

doc/devel/MEP/MEP28.rst

+======================
+
+Implementation of this MEP would eventually result in the backwards
+incompatible deprecation and then removeal of the keyword parameters


typo : removeal -> removal

NelleV · 2016-10-16T15:56:08Z

doc/devel/MEP/MEP28.rst

+``usermedians``, ``conf_intervals``, and ``sym``. Curosry searches on
+GitHub indicated that ``usermedians``, ``conf_intervals`` are used by
+few users, who all seem to have a very strong knowledge of matplotlib.
+A robust deprecation cycle should provide sufficient time for these


Considering we are have discussion about one release cycle versus two release cycle on the finance module right now, it might be worth being specific here.

story645 · 2016-10-16T16:27:10Z

doc/devel/MEP/MEP28.rst

+arguments and can easily be set to ``lambda x: x`` as a no-op when omitted
+by the user. The ``transform_in`` function will be applied to the data
+as the ``boxplot_stats`` function loops through each subset of the data
+pass to it. After the list of statistics dictionaries are computed the


minor: passed not pass

story645 · 2016-10-16T16:38:52Z

doc/devel/MEP/MEP28.rst

+This MEP seeks to dramatically simplify the creation of boxplots for
+novice and advanced users alike. Importantly, the changes proposed here
+will also be available to downstream packages like seaborn, as seaborn
+smartly allows users to pass arbitrary dictionaries of parameters through


So, a little unclear about scope delineation here. What does seaborn do above and beyond mpl? And how is this not creeping into seaborn's territory.

My thought is that these few changes should be implemented in such a way that seaborn users have access to them without seaborn needing to change at all. Shooting for simplicity and flexibility for users and downstream libraries

Hmm, so you want to maintain downstream use, but you plan to remove **kwargs. Is there verification that seaborn doesn't use those args? Granted, this seems like it'd be a pretty small PR on seaborn if it does change things with them..

seaborn uses very few parameters and just passes everything else from the user as **kws. Also instances of call matplotlib's boxplot API are below:

https://github.com/mwaskom/seaborn/blob/dd8ab4985eadba43c94542cfad56a78dcfefef31/seaborn/categorical.py#L468

https://github.com/mwaskom/seaborn/blob/dd8ab4985eadba43c94542cfad56a78dcfefef31/seaborn/categorical.py#L497

Next step would be to checkout yhat's python-ggplot

welp ggplot impliments their own boxplot drawer: https://github.com/yhat/ggplot/blob/5957a4db941be1da578ecc462d1f5b99f6d776ab/ggplot/geoms/geom_boxplot.py

In some ways that's probably good in that it means you don't have to worry about downstream support with ggplot.

you're absolutely right about that. i guess it just makes me feel insecure about our implementation.

story645 · 2016-10-16T20:04:05Z

doc/devel/MEP/MEP28.rst

+  2. v2.1.0 deprecate ``usermedians``, ``conf_intervals``, ``sym`` parameters
+  3. v2.2.0 make deprecations noisier (???)
+  3. v2.3.0 remove ``usermedians``, ``conf_intervals``, ``sym`` parameters
+  4. v2.3.0 deprecate ``notch`` in favor of ``shownotches`` to be consistent with


show_notches?

shownotches

Ugh, the inconsistency irks me. Like half the things, like conf_intervals put an _ between separate words and half, like shownotches don't.

I accept full responsibility for that

Any interest in deprecating args to move towards consistency?

Which did you have in mind? that was my intention with moving from notch to shownotches

moving stuff like shownotches to show_notches since that seems to be more pythonic.

I'd be open minded about that, my initial inclination is to leave out the underscores.

story645 · 2016-10-16T20:04:36Z

doc/devel/MEP/MEP28.rst

 With this approach, #2 depends and #1, and #4 depends on #3.

+There are two possible approaches to #2. The first and most direct would
+be to minor the new ``transform_in`` and ``tranform_out`` parameters of


not clear what "to minor" means here.

story645 · 2016-10-16T20:13:09Z

doc/devel/MEP/MEP28.rst

+be to minor the new ``transform_in`` and ``tranform_out`` parameters of
+``cbook.boxplot_stats`` in ``Axes.boxplot`` and pass them directly.
+
+The second approach would be to add ``statfxn`` and ``statfxn_args``


any chance of psuedocode like examples of the two approaches?

phobson · 2016-10-17T15:45:21Z

@story645 I've added some examples of what the final boxplot API might look like.

NelleV · 2016-10-17T16:39:09Z

doc/devel/MEP/MEP28.rst

+The second approach would be to add ``statfxn`` and ``statfxn_args``
+parameters to ``Axes.boxplot``. Under this implementation, the default
+value of ``statfxn`` would be ``cbook.boxplot_stats``, but users could
+pass their own function. Then `transform_in`` and ``tranform_out`` would


You are missing a backtick ` here.

NelleV · 2016-10-17T16:40:42Z

doc/devel/MEP/MEP28.rst

+
+Using matplotlib's stats function, this would look similar to this:
+
+.. python:


To be very annoying, ( 😄 ) could you add an example of how you would do this with the current API?
The difference would be more striking.

@NelleV great suggestion. done.

NelleV · 2016-10-17T18:34:52Z

My 2 cents is that we should merge this as "draft" quite quickly, and have discussion on the mailing list on the details.

phobson · 2016-10-17T19:50:37Z

@NelleV I'm going to clean this up tonight to make sure that the docs build (my rst is pretty poor). After that I'll remove the WIP and squash everything so far.

QuLogic · 2016-10-17T03:25:34Z

doc/devel/MEP/MEP28.rst

+Abstract
+========
+
+Over the past few releases, the ``Axes.boxplot`` method as grown in


QuLogic · 2016-10-17T03:25:48Z

doc/devel/MEP/MEP28.rst

+========
+
+Over the past few releases, the ``Axes.boxplot`` method as grown in
+complexity to support fully customizable artist styline and statistics


styline -> styling?
statistics -> statistical

QuLogic · 2016-10-17T03:26:48Z

doc/devel/MEP/MEP28.rst

+the results to ``Axes.bxp``, and pre-processing style information for
+each facet of the boxplot plots.
+
+This MEP will outline a path forward to roll-back the added complexity


QuLogic · 2016-10-17T03:27:19Z

doc/devel/MEP/MEP28.rst

+users to specify medians and confidence intervals for each box that
+will be drawn in the plot. These were provided so that avdanced users
+could provide statistics computed in a different fashion that the simple
+method provided by matplotlob. However, handling this input requires


matplotlob -> matplotlib

QuLogic · 2016-10-17T03:31:04Z

doc/devel/MEP/MEP28.rst

+could provide statistics computed in a different fashion that the simple
+method provided by matplotlob. However, handling this input requires
+complex logic to make sure that the forms of the data structure match what
+needs to be drawn. At the moment, that logic contain 9 separate if/else


contain -> contains

QuLogic · 2016-10-17T04:15:29Z

doc/devel/MEP/MEP28.rst

+With this approach, #2 depends and #1, and #4 depends on #3.
+
+There are two possible approaches to #2. The first and most direct would
+be to minor the new ``transform_in`` and ``tranform_out`` parameters of


minor -> mirror?

QuLogic · 2016-10-17T04:22:05Z

doc/devel/MEP/MEP28.rst

+The second approach would be to add ``statfxn`` and ``statfxn_args``
+parameters to ``Axes.boxplot``. Under this implementation, the default
+value of ``statfxn`` would be ``cbook.boxplot_stats``, but users could
+pass their own function. Then `transform_in`` and ``tranform_out`` would


Missing a backtick.
tranform_out -> transform_out

QuLogic · 2016-10-17T04:22:58Z

doc/devel/MEP/MEP28.rst

+parameters to ``Axes.boxplot``. Under this implementation, the default
+value of ``statfxn`` would be ``cbook.boxplot_stats``, but users could
+pass their own function. Then `transform_in`` and ``tranform_out`` would
+then be passes as elements of the ``statfxn_args`` parameter.


passes -> passed

QuLogic · 2016-10-18T04:35:54Z

doc/devel/MEP/MEP28.rst

+           if key != 'label':
+               s[key] = np.exp(value)
+
+   ax.bmp(stats, ...)


bmp -> bxp?

QuLogic · 2016-10-18T04:36:21Z

doc/devel/MEP/MEP28.rst

+Doing less
+----------
+
+Another obvious alternative would be to omitted the added pre- and post-


omitted -> omit

NelleV · 2016-10-18T16:05:48Z

@phobson the doc build is still not happening. If you have everything installed, the quickest by be to test on your computer with the parallelization option.

phobson · 2016-10-18T17:28:24Z

@NelleV Correct -- I need to include the API clarifications in my long comment above and then I'll get the doc builds working.

Sorry this is dragging out. Relearning transportation and structural engineering + the day job are filling my plate.

phobson · 2016-10-19T15:28:16Z

@QuLogic @NelleV @story645 I don't understand they appveyor failure, but the HTML doc build is working. In that sense, I think this is a ready for a "final" review to as a first draft for public discussion.

NelleV · 2016-10-19T15:38:13Z

Let's move the discussion to somewhere more public.

phobson · 2016-10-19T15:43:59Z

@NelleV thanks!

QuLogic · 2016-10-19T20:02:58Z

I guess it only fitting that @phobson sends the email to the mailing list.

phobson · 2016-10-19T20:07:12Z

@QuLogic I'll do that after #7309

mdboom added the status: needs review label Oct 16, 2016

NelleV reviewed Oct 16, 2016

View reviewed changes

story645 reviewed Oct 16, 2016

View reviewed changes

NelleV reviewed Oct 17, 2016

View reviewed changes

NelleV changed the title ~~WIP: first cut at boxplot MEP~~ WIP: first cut at boxplot MEP28 Oct 17, 2016

QuLogic reviewed Oct 18, 2016

View reviewed changes

WIP: first cut at boxplot MEP

5d2e6c3

phobson force-pushed the MEP-bxp branch from 688b9ab to 5d2e6c3 Compare October 19, 2016 06:04

phobson changed the title ~~WIP: first cut at boxplot MEP28~~ Draft version of MEP28: Simplification of boxplots Oct 19, 2016

NelleV merged commit accd403 into matplotlib:master Oct 19, 2016

mdboom removed the status: needs review label Oct 19, 2016

phobson deleted the MEP-bxp branch October 19, 2016 15:43

QuLogic added this to the 2.1 (next point release) milestone Oct 20, 2016

		:local:


		This MEP template is a guideline of the sections that a MEP should


		Using matplotlib's stats function, this would look similar to this:

		.. python:

Uh oh!

Conversation

phobson commented Oct 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phobson commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV commented Oct 17, 2016

Uh oh!

phobson commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment