ENH: Fix `by` in DataFrame.plot.hist and DataFrame.plot.box
State: closed

- [x] closes #15079
- [x] xref:  #11053, #28177
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Visualization
description: plotting

Comments:
@charlesdong1991 Thanks for the PR! What issue does this relate to/close?
Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:













There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2021-07-12 06:22:22 UTC
ahh, sorry, it's still in progress, so i didn't add that

Added! @simonjayhawkins 
@TomAugspurger I know this looks like a big change, and I am not sure if this is the right way. RIght now, here what I am doing is allowing plotting for `by` arguments, and this can lay the foundation for getting rid of `df.hist()` as well as `df.boxplot` IMO for the near future. The output is slightly different, since `df.plot` now uses pandas way of plotting, while `df.hist` now almost shows the matplotlib plots IIUC.

Here are the plots looking like in different scenarios, and looks not affect other tests in my local tests:

![Screen Shot 2019-09-12 at 10 58 30 PM](https://user-images.githubusercontent.com/9269816/64821118-49c43080-d5b2-11e9-8cc0-81f9ab7b5d68.png)

![Screen Shot 2019-09-12 at 10 58 51 PM](https://user-images.githubusercontent.com/9269816/64821127-4f217b00-d5b2-11e9-9ee0-ffa2ec144ea8.png)

I would appreciate a lot if I could hear some feedbacks from you, and maybe correct my direction if you think this change is not proper. I will do some further cleaning and add more tests if you think this is the way to go.

Thanks!

emm, I would appreicate a lot if anyone can help me a bit with it, and let me know why the error was caused? I had similar experience before, but this time I didn't do `import pandas as pd`, instead, i used `from pandas import blabla`... but still get error like last time... thanks a lot in advance! @datapythonista @WillAyd @TomAugspurger 
can't look in detail now, but the failure seems to be because your code is not raising a warning anymore

CC @martinagvilas (I think you were having a look at this issue too, or something similar)
Can you reproduce the failures locally? `tests/plotting/test_converter.py`. If there is anyway to split this PR up that might help it getting merged. @charlesdong1991.

Also see this in CI:
`AttributeError: module 'pandas.plotting._matplotlib' has no attribute 'converter'`
@charlesdong1991 is this still active? Can you merge master?
i will give another try this weekend to see if tests could be passed. @WillAyd 
Can we merge master + get the test failures fixed? Thanks for the effort so far - @charlesdong1991 
well, finally GREEN! 😅  I think i could summarize a bit this PR since it's been here for quite a long while.

Aim: the purpose of this PR is to fix `by` argument for `hist` plot, since this is not implemented at all, and this can be seen as the first step to standardize plot api (to make it `df.plot(kind='hist')` and deprecate `df.hist()` in the near future.

Result: as you could see below, the functionality is implemented, but they look a bit different. However, I checked and found this is just how pandas plot differentiates from hist plot. In the third picture, you could see that with current setup, when we want to plot subplots for two columns, using `df.plot.hist` will generate plots one below each other, while the layout for `df.hist` is different, and will plot first two plots on the same row (likely the `ncols` is set to 2 while `df.plot.hist` is 1), if there are more subplots, then start with new row.

![Screen Shot 2019-09-12 at 10 58 30 PM](https://user-images.githubusercontent.com/9269816/64821118-49c43080-d5b2-11e9-8cc0-81f9ab7b5d68.png)

![Screen Shot 2019-09-12 at 10 58 51 PM](https://user-images.githubusercontent.com/9269816/64821127-4f217b00-d5b2-11e9-9ee0-ffa2ec144ea8.png)

**The current difference when plotting multiple subplots is already there:**
![Screen Shot 2020-01-04 at 3 21 01 PM](https://user-images.githubusercontent.com/9269816/71767004-e8302500-2f06-11ea-85d1-3382754b7eef.png)

Difference: the biggest difference is the layout is different, but seems the difference is common in current `df.plot` apis.

Pros of `df.plot.hist`: 
1. with `df.plot.hist`, we could plot subplots in the same figure by setting `subplots=False`, while we could not do it with `df.hist`, so for instance, in the case above, `df.hist(column=['a', 'b'])` will always have two subplots
2. `df.plot.hist` could have `legend` and `labels` in the plot while `df.hist` does not have it now
3. `sharex` seems work more correct in `df.plot.hist`

My proposal:
1. I could first implement `by` for `df.plot.hist` and `df.plot.boxplot` and allow pandas users to try and test them, and in the meanwhile, keep the current `df.hist` and `df.boxplot` for one more version.
2. Once bugs with `df.plot` are less, and we are confident of using `df.plot.hist` and `df.hist.boxplot`, then could deprecate `df.hist` and `df.boxplot`
Hi, @datapythonista and @jreback 

I have simplified a lot of the codes, and add extensive tests (kind of duplicates the tests we have already for `df.hist`). 

Basically in this PR it does:
1. add `by` in core.py to reconstruct dataframe to be plotting grouped data for `hist`, and this part can be reused for `boxplot` as well, once this PR can be merged, I will come up with another PR to implement `by` for `df.plot.boxplot`
2. Adjust bins calculation and `_make_plot` method to correctly plot figures given `by`
3. Add many tests for `df.plot.hist` just like we have for `df.hist` for `by` argument.

One thing I have to mention is the color setting, this seems not working yet, so need to use default color setting. And it is a little bit complex in `by` situation, and would like to have a followup PR to figure it out if you dont mind. For the rest, it seems good to me!

Feel free to review and provide your opinions! Thanks!

thanks a lot for your reviews @datapythonista , appreciate a lot!

I changed code based on your comments, and also add whatsnew note and add example in `plot.hist`, feel free to give another look!

gentle ping @datapythonista @jreback for follow-up ^^ would appreciate a lot!
gentle ping @datapythonista for further feedback, thanks a lot! ^^
@jreback @WillAyd @TomAugspurger some follow-up reviews are very appreciated 👍 
@WillAyd thanks very much for your reviews!! I think i addressed/replied all of them, feel free to give a follow-up! ^^
nice idea @alimcmaster1 shorted
@jreback @datapythonista @WillAyd any follow-up comments?
very gentle ping for reviews, thanks! ^^ @jreback @WillAyd @datapythonista 
@jreback thanks for reviewing, i changed/replied all your reviews. pls take another look for opinions
@jreback sorry to push for a review, some feedbacks would be appreciated.
> Just to make sure we're in the same page. DataFrame.hist() already does what you're implementing here, I guess you check how it is done.

yeah, `df.hist()` bascially has what i have done here, but the implementation is slightly different because `df.plot()` uses `ax` and `data` in a slightly different way, while `df.hist()` basically directly transform the data to be used for `ax.hist()`. And that's why i referenced some ideas in the current `df.hist()` and tried to do it with `df.plot(kind='hist')`. And the final goal here is make `df.plot.hist` ready and be prepared to deprecate `df.hist()` in the future.

I don't know a case now, but I guess DataFrame.plot() already has some cases where we generate more than one plot in the output figure. If you can find a case, and see how that is implemented, that's exactly what we want here.

thanks, I checked it, indeed, for instance, `bar` plot has the case where we generate more than one plot in the output figure, and they use `_iter_data` so that each column will be plotted separately (e.g. to have different colors to differentiate), however, this `hist` is slightly different, because we need to generate more than one plot in multiple output figures. I will try again to see if anything could be improved and simplified to use less code here.

thanks again for your reviews and comments, very appreciate! @datapythonista And pls let me know if you think we are not on the same page, so taht I could adjust my direction a bit to get this work done 👍 

I will in the meantime make changes based on your comments, and will let you know once it is done.
Can you share the example of `bar` that is generating more than one plot? Or you mean having bars from different columns in the same plot? Will have a look and see if I can see an example of that, if we plot several plots in the same figure somewhere, that should be quite reusable for this.
@charlesdong1991 This is using the `DataFrame.plot` and plotting more than one plot:
```python
import pandas

portfolio = pandas.DataFrame({'2019': [500, 1000, 300],
                              '2020': [200, 2500, 400]},
                             index=['AAPL', 'MSFT', 'FB'])

portfolio.plot.pie(subplots=True, figsize=(8, 4))
```
I think we should implement the `by` for `DataFrame.plot.hist()` in the same way.
> I think we should implement the by for DataFrame.plot.hist() in the same way.

> can we just always yield dict-of-Series? its much simpler

Many thanks for your reviews, really appreciate it!!. 
i think it is different than `df.plot.pie`, because `df.plot.pie` only take values from one column for each `ax`. However, with `df.plot.hist`, the `column` arguement can take a two or more elements list, so values from more than two columns will be yielded as input for plotting methods, and in this case, we do not want to yield `dict-of-Series`. For current other plotting methods, we only take values from one column to apply, therefore, it works e.g. for `pie` plot. However, adding `by` for hist could change the situation, because if we only yield `dict-of-Series` it will only work for examples like `df.plot.hist(column='col1', by='col2')` or `df.plot.hist(column='col1', by=['col2', 'col3'])`, but not for cases like `df.plot.hist(column=['col1', 'col2'], by=['col3'])`, `df.plot.hist(column=['col1', 'col2'], by=['col3', 'col4'])`, and this is the reason I need to change the `_iter_data` which i think might be the cleaner solution than changing other places.  @datapythonista @jreback 

Do you have any other suggestions where it could be changed that could make the change clearer? 
Hi, @jreback @datapythonista @WillAyd 

I have read through all your comments and made changes again to align with them. I think previously the biggest argument is on `_iter_data` which should be remained unchanged. However, for `by` case, some changes need to be made to get it work (this is different than `pie` and others that each time only iterates one single column, described here: https://github.com/pandas-dev/pandas/pull/28373#issuecomment-599228003). Therefore, I moved processing part out, and assign the processed data to `data` argument in `_iter_data` method, and this way we keep `_iter_data` untouched, and also have a clearer view on what is changed on data if `by` is assigned.

In the meantime, I also changed some documentations/details according to @datapythonista comments. I feel this PR should be clearer and more readable now to get review process speed up a bit.

Appreciate a lot for further reviews and thanks for all your efforts on existing reviews!! 
@datapythonista Many thanks for your reviews, really appreciate a lot!

I have added clearer and more thorough docs and example and tests, and hopefully could the review easier and code more understandable.
any followups on it? @datapythonista @jreback @WillAyd ^^ thanks!
maybe let's try to push this PR once again? thanks! ^^

@datapythonista @WillAyd  i think comments have been addressed.