GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — ENH/REF: Overhauled boxplots by phobson · Pull Request #2643 · matplotlib/matplotlib

Conversation

@phobson

@phobson phobson commented Dec 3, 2013

Copy link
Copy Markdown
Member

In a nut shell here is what has happened:

  1. Created a function (cbook.boxplot_stats) that take any data that axes.boxplot could handle and generates a list of dicts containing the statistics needed to draw a set of boxplots.
  2. Then I created a new method (axes.bxp) that accepts dictionaries from cbook.boxplot_stats and allows the user to customize how the plots are rendering. In other words, dictionaries of style **kwargs can be passed configuring the style of the boxes/whisker/caps, the outliers, the medians, the means (new functionality).
  3. The new bxp method also allows the user to toggle the drawing of any element of the boxplot via **kwargs (i.e., shownotches, showboxes, showfliers, showmeans, showcaps).
  4. The new bxp looks for a label key in the dictionaries. If it's available, it'll use that as the x-tick label. Otherwise, the labels default to the boxplot's position.
  5. And finally, the original boxplot method is functionally unchanged, but now it relies on cbook.boxplot_stats and axes.bxp to do all the work.
  6. Added a bunch of tests

Potential remaining items:

  1. Well, I definitely need to draft up a good example to show all of this off. That's coming shortly (pending feedback on the existing portions of the PR).
  2. We might want to expose the full capability of bxp in boxplot (i.e., the style and show* kwargs). Currently, boxplot's API/capabiltiy is unchanged. I'm fine with this but I'll need to pretty much duplicate all of the bxp tests to make sure nothings gets lost in translation, (done)
  3. Rerunning boilerplate.py didn't seem to pick up on bxp. Do I need to clear it out first?
  4. We might want to add boxplot_stats to pyplot and pylab. (not gonna happen)
  5. And it looks like boilerplate.py added a bunch of unicode strings to the call sigs and now the 3.2 build is failing (dang it). (fixed that)

This PR addresses the following issue (or allows users to address them themselves):

  1. New boxplot features #1199
  2. New features for boxplot #217
  3. Adds option to plot average in boxplot, besides the median #2520
  4. Boxplot: allow whiskers to always cover entire range #1455

There is all #1027 out there. While we don't address that issue directly. I personally agree with matplotlib's approach to boxplots that brings up that issue. In other words, that's a feature, not a bug, IMO

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
phobson added 10 commits December 2, 2013 10:39
@phobson
REF/TST: split up boxplot function into a calculator (cbook.boxplot_s…
8bf8653 
…tats) and a drawer (axes.bxp).

Existing function now relies on these two functions.
@phobson
BUG: fixed cbook tests for Python 2.6
b429dcf
@phobson
ENH: allow for custom patch artist and labels to the boxplots
04937b5
@phobson
WIP: more tests and add option to toggle box
88435bc 
TST: added tests for all of bxp and more boxplot tests

TST: fixed boxplot baseline image

TST: added baseline images for bxp
@phobson
DOC/REF: added docstring for axes.bxp and refactored all references t…
1002913 
…o outliers as fliers
@phobson
REF/TST: update boxplot_stats test with outliers changed to fliers
614736d 
ENH/TST: boxplots stats now takes a labels kwarg

MNT: minor PEP8 whitespace fix
@phobson
MNT: reran boilerplate.py
ec12ef9
@phobson
DOC: added blurb in changelog
ffdbd7d
@phobson
MNT: got rid of build-breaking u'' strings in pyplot
987893c
@phobson
MNT: fixed changelog whitespace
2562b68
@tacaswell

tacaswell commented Dec 3, 2013

Copy link
Copy Markdown
Member

I don't think it is worth re-fitting boxplot to replicate the new functionality. It is important to keep it's interface as-is for backwards compatibility, but point users who want the new features to the new function (which is where the examples come in ;) )

You need to add it to the list _plotcommands in boilerplate.py to get picked up.

There was discussion about doing away with pylab in the last hangout and I don't think it is a good fit in pyplot.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson

phobson commented Dec 3, 2013

Copy link
Copy Markdown
Member Author

@tacaswell To be fair, I think we can add the new stuff to boxplot without breaking anything. We just have to add bxp's kwargs to boxplot's call signature, document them, and pass them directly to bxp (which already happens in a very limited fashion),

Thanks for the other tips. I'll look into them soon.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson phobson mentioned this pull request Dec 3, 2013
Boxplot: allow whiskers to always cover entire range #1455
Closed
phobson added 5 commits December 3, 2013 13:21
@phobson
ENH: cbook.boxplot_stats can now set the whiskers to -range- (min/max…
7f937f9 
…) or specified percentiles. If IQR is zero, min/max are used
@phobson
TST: added tests and baseline images for new whisker behavior
fc80458
@phobson
ENH: added remaining bxp kwargs to boxplot
85d91df
@phobson
DOC: expanded boxplot_stats docstring
d079ad2
@phobson
DOC: added examples
5a9f610
tacaswell
tacaswell reviewed Dec 13, 2013
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py

@tacaswell tacaswell Dec 13, 2013

Copy link
Copy Markdown
Member

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.

Would it be better to have this logic shuffle/flip the data and only have the plotting once rather than creating inner functions which do both the data munging and the plotting (sorry if it become obvious later why you did it this way).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@phobson phobson Dec 13, 2013

Copy link
Copy Markdown
Member Author

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.

@tacaswell I could definitely rearrange that as you describe. this is just an vestige of the original boxplot code. luckily I think i have enough tests now that I'll know if I break something :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@efiring efiring Jan 19, 2014

Copy link
Copy Markdown
Member

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.

@tacaswell, my first reaction probably would be like yours, but looking farther down in the code, I think that using these local functions is actually the cleanest way to encapsulate the axis flip.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tacaswell

tacaswell commented Dec 13, 2013

Copy link
Copy Markdown
Member

I left a few picky comments that other should comment on before you do anything about.

I am also not sure about the * in the doc strings.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson

phobson commented Dec 13, 2013

Copy link
Copy Markdown
Member Author

@tacaswell thanks for the feedback. As you suggested, I'll wait for more comments. The *s in the docstring are also vestiges of the original code. I think it'll be safe to proceed on converting the docstrings to the numpy format.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson phobson mentioned this pull request Dec 13, 2013
ENH: Add ohlc / candlestick plotting option for DataFrame pandas-dev/pandas#783
Closed
@tacaswell

tacaswell commented Dec 13, 2013

Copy link
Copy Markdown
Member

Your comment re pandas does raise the interesting point that matplotlib.financial will currently make candlestick plots which should maybe be re-written using this. On the other hand, that module will be pulled out in 1.5.

This looks like a really nice PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson

phobson commented Dec 13, 2013

Copy link
Copy Markdown
Member Author

Happy to take a look at the candlestick, as it should be pretty easy. However, that'll have to happen in a separate PR if the Repo Collabs decide it's worth it. Can you provide some guidance with my examples? Namely, do I need to include the output figures or does the doc-building process take care of that?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tacaswell

tacaswell commented Dec 13, 2013

Copy link
Copy Markdown
Member

pretty sure the doc-build process takes care of that, but I have not done much with it. The easy thing is to run the doc-build locally and see if it takes care of it.

Yes, I was not suggesting you do the candlestick in this PR (or even that you should do it). It would be good to find a person who does finance to take care of that module/toolkit, but that is irrelevant for this PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson
DOC: impoved example boxplot figure layout and confirmed that they bu…
13937e9 
…ild with docs
@phobson

phobson commented Jan 2, 2014

Copy link
Copy Markdown
Member Author

@tacaswell
Just chiming in to say that I built the docs and the new examples were included. Any other feedback, suggestions, requests, etc are welcome!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tacaswell

tacaswell commented Jan 2, 2014

Copy link
Copy Markdown
Member

@phobson This needs a re-base.

It looks good to merge to me, but I don't want to push the button on this much new code without someone else looking at it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson

phobson commented Jan 2, 2014

Copy link
Copy Markdown
Member Author

@tacaswell do i need to rebase to squash the commits, to sync with current master, or both?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
efiring
efiring reviewed Jan 18, 2014
View reviewed changes
Comment thread doc/users/whats_new.rst Outdated

@efiring efiring Jan 18, 2014

Copy link
Copy Markdown
Member

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.

Maybe "each of the artists"? This is still not very clear.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
phobson added 5 commits January 18, 2014 16:27
@phobson
cleanup up typos and minor tweaks based on dev team input
da97eab
@phobson
cleaned up whats_new (thanks, efiring)
437f5c2
@phobson
better shaping checking logic
f26a402
@phobson
boxplot and bxp docstrings to a more numpy-like format
f2aea5b
@phobson
fixed usermedian shape logic (again)
9b0118f
@tacaswell tacaswell mentioned this pull request Jan 22, 2014
boxplot treats iterables differently by type #2539
Closed
@phobson
reran boilerplate.py
64b1196
tacaswell added a commit that referenced this pull request Jan 24, 2014
@tacaswell
Merge pull request #2643 from phobson/refactor-boxplots
cc6ae3d 
ENH/REF: Overhauled boxplots
@tacaswell
tacaswell merged commit cc6ae3d into matplotlib:master Jan 24, 2014
@tacaswell

tacaswell commented Jan 24, 2014

Copy link
Copy Markdown
Member

@phobson Merged! Thanks for this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson

phobson commented Jan 24, 2014

Copy link
Copy Markdown
Member Author

@tacaswell wooo! thanks for the guidance and patience
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@phobson
phobson deleted the refactor-boxplots branch January 24, 2014 06:33
@takluyver takluyver mentioned this pull request Jan 25, 2014
Docs build failure #2766
Closed
This was referenced Jan 27, 2014
New features for boxplot #217
Closed
Possible bug in boxplot() #1027
Closed
@tacaswell tacaswell mentioned this pull request Jan 25, 2015
Hist Plot Normalization should allow a 'Per Bin' Normalization #4038
Closed
@QuLogic QuLogic mentioned this pull request Feb 10, 2018
boxplot regression testing #3572
Closed
@has2k1 has2k1 mentioned this pull request Aug 3, 2019
Implementing geom_hex has2k1/plotnine#296
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.4.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@phobson @tacaswell @Sjlver @efiring
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.