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

BUG: Boxplot to return axes, not dict #4472

Closed
wants to merge 3 commits into from
Closed

BUG: Boxplot to return axes, not dict #4472

wants to merge 3 commits into from

Conversation

phobson
Copy link

@phobson phobson commented Aug 5, 2013

Fixes #4264.

@jreback
Copy link
Contributor

jreback commented Aug 23, 2013

  • can you hook up travis
  • test
  • release notes entry

is this consistent with #4020 (I believe that's the point here!)

@phobson
Copy link
Author

phobson commented Aug 29, 2013

@jreback

  • Hooked up Travis
  • I don't think there's much testing to add here. I'm not sure we even want to keep the last commit i just pushed. I think it has much larger implications about all of the plotting functions that we really don't want to deal with in the simple PR that deals with just a simple issue
  • Release notes coming after we decide on the direction to take re: my last "TST" commit

@@ -1068,7 +1067,7 @@ def assert_is_valid_plot_return_object(objs):
'type encountered {0!r}'
''.format(el.__class__.__name__))
else:
assert isinstance(objs, (plt.Artist, tuple, dict)), \
assert isinstance(objs, plt.Axes), \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phobson This should be kept as plt.Artist since not all plotting functions return Axes instances.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phobson Can you remove the dict check here, since boxplot was the only function returning a dict?

@jreback
Copy link
Contributor

jreback commented Aug 29, 2013

@cpcloud can u take a look

@cpcloud
Copy link
Member

cpcloud commented Aug 29, 2013

Looks okay to me, modulo what I said in the outdated diff.

@jreback Do you think it's worth noting in the docs how to recreate the dict that's returned as of now? matplotlib returns a dict as well so users coming from there might expect a dict.

@phobson
Copy link
Author

phobson commented Aug 29, 2013

@cpcloud
The more I think about this, the more I think that boxplot should return the ax to be consistent with the other functions, but there is value in the dict of Artists as well. Namely, boxplots of out of the -- uh-- box are pretty ugly. I always keep a function like this handy:

def formatBoxplot(bp, color='b', marker='o', linestyle='-'):
    # format the box itself
    for box in bp['boxes']:
        plt.setp(box, color='k', linewidth=0.75, zorder=4)

    # format the medians
    for med in bp['medians']:
        plt.setp(med, linewidth=1.00, color=color, linestyle=linestyle, zorder=2)

    # format the whiskers
    for whi in bp['whiskers']:
        plt.setp(whi, linestyle='-', color='k', linewidth=0.75, zorder=4)

    # format the caps on top of the whiskers
    for cap in bp['caps']:
        plt.setp(cap, linewidth=0.75, zorder=4)

    # format the outliers
    for fli in bp['fliers']:
        plt.setp(fli, marker=marker, markersize=4, zorder=4,
                 markerfacecolor='none', markeredgecolor=color)

Any thoughts on returning both the Axes and the dict.

@cpcloud
Copy link
Member

cpcloud commented Aug 29, 2013

@phobson I was thinking something similar after glancing at the matplotlib source, sigh. I'm not sure there's a point to return both the axes and the dict since you can get the axes easily with gca() or get it from one of the lines in the dict (although this annoying so maybe return both might be better here)

@cpcloud
Copy link
Member

cpcloud commented Aug 29, 2013

now that i think about it, i think both is an okay compromise

@phobson
Copy link
Author

phobson commented Aug 29, 2013

Since Travis is failing and I'm not at a machine where I can easily build this, I'll have to pick this up again later.

@jreback
Copy link
Contributor

jreback commented Sep 26, 2013

@phobson ping!

@phobson
Copy link
Author

phobson commented Nov 3, 2013

Get folks. Sorry for the delay. When started poking around the source and tests, this was a lite more complicated than I anticipated. I hope to get to this soon. (Maybe tonight, or by the end of the week.)

Paul Hobson
Sorry if this is unintelligible. I'm on my phone.

On Wed, Sep 25, 2013 at 4:59 PM, jreback notifications@github.com wrote:

@phobson ping!

Reply to this email directly or view it on GitHub:
#4472 (comment)

@seclinch
Copy link

The documentation seems to suggest that pandas.DataFrame.boxplot already returns a matplotlib.axes.AxesSubplot object -- but I don't believe this to be the case?

Perhaps the documentation could be updated to indicate that boxplot() currently returns a dict?

@jtratner
Copy link
Contributor

@seclinch - it'd be great if you could submit a pull request with the docs change. (which we'll revert once this current pull request is merged)

@jreback
Copy link
Contributor

jreback commented Feb 16, 2014

@cpcloud is this correct?

@phobson maybe a test to validate that the box plot is in fact returning a type of Axis (and not something else?)

@jreback
Copy link
Contributor

jreback commented Mar 9, 2014

@phobson can you rebase this

@jreback jreback modified the milestones: 0.15.0, 0.14.0 Mar 22, 2014
@jreback
Copy link
Contributor

jreback commented Apr 5, 2014

@phobson can you rebase.... @cpcloud pls review

@jreback
Copy link
Contributor

jreback commented Apr 9, 2014

@TomAugspurger @cpcloud

what do you think?

@cpcloud
Copy link
Member

cpcloud commented Apr 9, 2014

@phobson can you add a doc saying how to get the dict back? Then I think good to go.

@jreback
Copy link
Contributor

jreback commented Apr 9, 2014

does this fix #4636 as well?

@phobson
Copy link
Author

phobson commented Apr 9, 2014

I don't think this is ready yet. I'll have some bandwidth on friday to look
into it though. Last I remember I was having trouble groking the tests.

On Wed, Apr 9, 2014 at 5:56 AM, jreback notifications@github.com wrote:

does this fix #4636 #4636 as
well?

Reply to this email directly or view it on GitHubhttps://github.com//pull/4472#issuecomment-39960179
.

@jreback jreback modified the milestones: 0.14.0, 0.15.0 Apr 9, 2014
@cpcloud
Copy link
Member

cpcloud commented Apr 9, 2014

Let us know if we can help.

@cpcloud
Copy link
Member

cpcloud commented Apr 9, 2014

There's a hacky plot testing function that special cases checking the return value type of boxplot. You'll have to remove the lines that check for a dict and then the tests should pass.

@jreback
Copy link
Contributor

jreback commented Apr 22, 2014

@phobson status on this?

@TomAugspurger @cpcloud

@jreback
Copy link
Contributor

jreback commented Apr 28, 2014

@phobson ok....going to do this for 0.14.

@TomAugspurger
Copy link
Contributor

See @jseabold's comment here #4264 (comment)

I agree that box plot should always return an axes (array of axes if subplots). An optional kwarg return_dict=False (default) to return a dictionary in addition to the axis will allow for more formatting.

@jreback
Copy link
Contributor

jreback commented May 10, 2014

superseded by #7096

@jreback jreback closed this May 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataFrame.boxplot returns a dict instead of axes
6 participants