-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
API: Return axes from boxplot #7096
Conversation
FYI I don't see any uses of |
i think should return the dict by default ... unless there's a reason to break the API more than necessary (i think ok to break by returning the named tuple) but defaulting to not return the boxplot anymore seems too much ... not that big of a deal |
@TomAugspurger I think your soln seems reasonable. @cpcloud I think returning BOTH breaks much more here; with @TomAugspurger PR, you can get both just by adding a kw, otherwise it doesn't break |
Shall we have a vote?
2, 5, and 6 will break peoples' code. I vote for 5 or 6. |
I should note that this only applies when |
5 gets my vote also, FWIW |
👍 on 5 from me too. |
@TomAugspurger since current a dict is returned, and proposal is for 5 returning an just thinking that user upgrades and doesn't read the API changes in whatsnew (doesn't everyone??? ) ... lol |
5 is ok I suppose. I will only offer as anecdotal evidence that whenever I update my pandas, parts of my large projects inevitably break and it's a huge time-sink to track things down (happened as recently as yesterday). Breaking code is "bad." I've been busy lately, so I've just stopped trying to report these types of things. OTOH, this is a bit of a wart, and it should be pretty clear what's going on. If you decide on 5, then I'd issue a warning for a few releases to help people figure out what's going on. Another option, that's a pain, but is the way we went with statsmodels in a few cases. Keep returning a dict, so as not to break people's code and add |
Thanks @jseabold. Should we try to deprecate / warn for a release? I could add a |
That sounds reasonable to me, but I'll defer to whatever others want to do. API breakage isn't the end of the world in this case. As has been pointed out this isn't exactly a subtle change. IMO deprecation is just a good software habit for such a heavily used package. |
I am +1 on (eventually) returning the axis by default. And +1 on having a deprecation cycle, keeping the default for now but raising a warning this will change in the future. |
I am with @jorisvandenbossche on first point but +0 on deprecation cycle, I think this would break loudly and this has been a wart for a long time |
so since majority want deprecation @TomAugspurger why don't we use the |
OK. So to summarize:
And I should note that all of this goes out the window if I pretty much have this done btw. I'm just in the midst of a messy rebase. |
@TomAugspurger sounds good to me; create a new issue for DEPR review in 0.15 (the other is for 0.16); just because this has been outstanding for SO long |
good summary! |
Added to issue for 0.15 deprecations: #7136 What I just pushed up should implement the keep same default for now, but deprecate and warn strategy. I also made a shared docstring for Finally, I was getting an error on one of the tests that checked a figsize since I change my default figsize in my |
fontsize : int or string | ||
rot : label rotation angle | ||
figsize : A tuple (width, height) in inches | ||
grid : Setting this to True will show the grid | ||
layout : tuple (optional) | ||
(rows, columns) for the layout of the plot | ||
return_dict : {'axes', 'dict', 'both'}, default 'dict' |
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.
return_type here?
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.
yes
Oh my, the return type gets more complicated. I also should look into changing the repr of a named tuple. This isn't great.
|
Fixed that wrong kwarg in the docstring and added a note about the return type from a grouped boxplot. This should be ready |
looks good |
@@ -258,6 +261,10 @@ Deprecations | |||
Use the `percentiles` keyword instead, which takes a list of percentiles to display. The | |||
default output is unchanged. | |||
|
|||
- The default return type of :func:`boxplot` will change from a dict to a matpltolib Axes | |||
in a future release. You can use the future behavior now by passing ``return_type='dict'`` |
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.
shouldn't this be 'axes'?
Thanks. On May 16, 2014, at 3:26 PM, "jreback" <notifications@github.commailto:notifications@github.com> wrote: merged via a0692behttps://github.com/pydata/pandas/commit/a0692be4ed56ab6c2eb91462a03d992d48ecb915 — |
@TomAugspurger this is deprecated; shall we fix for 0.17.0? |
Part of pandas-dev#6581 Deprecation started in pandas-dev#7096 Changes the default value of `return_type` in DataFrame.boxplot and DataFrame.plot.box from None to 'axes'.
* DEPR: Change boxplot return_type kwarg Part of #6581 Deprecation started in #7096 Changes the default value of `return_type` in DataFrame.boxplot and DataFrame.plot.box from None to 'axes'. * API: Change faceted boxplot return_type Aligns behavior of `Groupby.boxplot` and DataFrame.boxplot(by=.) to return a Series.
Closes: #4264
Will go in place of #4472
In #985 we intentionally decided to have boxplot return a dict with the Lines of the boxplot.
I've added a kwarg
return_dict
. By default,return_dict
is False. In this case only amatplotlib.axes
is returned. Whenreturn_dict
is True, we should eithermatplotlib.axes
and the dict in a namedtupleBoth of these are API breaking. Right now I've got both in the named tuple, but I may change that to just return just the dict. If we just return the dict, the only change people will have to make to their code is to add
return_dict
to True in their boxplot calls. Thoughts?cc @jseabold, @fonnesbeck