move pivot_table doc-string to DataFrame
State: closed

instead of pinning it on over pin `core.reshape.pivot`

At the moment `core.reshape.pivot.pivot_table` is pinned to `DataFrame` by writing `DataFrame.pivot_table = pivot_table`.  This PR defines `pivot_table` directly in `DataFrame` and then calls to the function.  It's the same pattern already used by `DataFrame.pivot`.

I think this is the last `DataFrame` method pinned on to `DataFrame` this way.  Two more left in `Series`

 - [ ] closes #xxxx
 - [ ] tests added / passed
 - [x] passes ``git diff upstream/master -u -- "*.py" | flake8 --diff``
 - [ ] whatsnew entry


Labels:
name: Docs
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
Hello @jbrockmendel! Thanks for updating the PR.





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on August 09, 2017 at 01:25 Hours UTC
> why are you trying to move back?

Because pinning methods/attributes on from afar makes these classes harder to reason about.  In a non-trivial set of cases, these classes are altered at import time with `hasattr` checks, making the code brittle to changes in import order (#16931, #16959).  There is One Obvious Way To Do It, and that's to define the method in the class definition.

I'll take a look at the old discussion and try to understand the reasoning.
+1 on this change. I would only keep the docstring with the implementation, and add it on the dataframe one.


@jorisvandenbossche you are not remembering: https://github.com/pandas-dev/pandas/pull/15521/files


> I would only keep the docstring with the implementation, and add it on the dataframe one.

That was my initial inclination too, but then attaching the docstring to `DataFrame.pivot_table` causes a circular import mess.  `core.reshape.pivot` imports `DataFrame` and a bunch of other modules, some of which also import `DataFrame`.  I'd like to de-tangle this, but trying to do that in this PR would snowball.
@jbrockmendel ``pandas/core/frame.py`` is WAY too long. Functions are often defined externally to better group them on purpose.

Moving things back defeats the purpose of delegation and simplicity. The maxims of python need to be balanced with practicality.

This is an anti-change to all pandas philosophy.
> `pandas/core/frame.py` is WAY too long

Very much agreed.  

How about this: I'll implement @jorisvandenbossche 's suggestion of putting the docstring with the implementation in `core.reshape.pivot` (or in `_shared_docs`).  Then defining the method directly in `frames.DataFrame` will take up 9 lines (4 if you don't count wrapping).  While I'm at it, I'll do the same for `DataFrame.pivot.__doc__`, `_shared_docs['melt']`, and `DataFrame._agg_doc` (which I expect doesn't belong in the namespace anyway).

Then `core.frames` is a few hundred lines shorter, tangled imports in `core.reshape` are replaced with `ABCFoo` imports, and the original goal of the PR is accomplished.
> @jorisvandenbossche you are not remembering: https://github.com/pandas-dev/pandas/pull/15521/files

Not sure what you are trying to argue .. as the PR did exactly the same as this PR is now doing for another method? 
> Moving things back

@jreback this PR is *not* moving the actual *implementation* back, it is just changing the way how the the method is added to the DataFrame class to a more explicit and consistent (with other methods) way.
The test failures are being caused by a problem with `ABCIndex`.  I've fixed them for this PR and am about to open an issue for the underlying problem.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=h1) Report
> Merging [#17174](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/929c66fd74da221078a67ea7fd3dbcbe21d642e0?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17174/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17174      +/-   ##
==========================================
+ Coverage   90.99%      91%   +<.01%     
==========================================
  Files         162      162              
  Lines       49508    49514       +6     
==========================================
+ Hits        45052    45061       +9     
+ Misses       4456     4453       -3
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.78% <100%> (+0.02%)` | :arrow_up: |
| #single | `40.26% <25%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `95.34% <100%> (+0.04%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.66% <100%> (-0.1%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `65.05% <0%> (+1.81%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=footer). Last update [929c66f...fce2a38](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=h1) Report
> Merging [#17174](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/929c66fd74da221078a67ea7fd3dbcbe21d642e0?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17174/graphs/tree.svg?width=650&height=150&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17174      +/-   ##
==========================================
- Coverage   90.99%   90.98%   -0.01%     
==========================================
  Files         162      162              
  Lines       49508    49521      +13     
==========================================
+ Hits        45052    45059       +7     
- Misses       4456     4462       +6
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.77% <100%> (+0.01%)` | :arrow_up: |
| #single | `40.27% <55%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.71% <100%> (-0.05%)` | :arrow_down: |
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `95.4% <100%> (+0.1%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYmFzZS5weQ==) | `96% <0%> (-0.21%)` | :arrow_down: |
| [pandas/plotting/\_core.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb3JlLnB5) | `82.68% <0%> (-0.13%)` | :arrow_down: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `95% <0%> (-0.05%)` | :arrow_down: |
| [pandas/core/sparse/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL2ZyYW1lLnB5) | `94.26% <0%> (-0.02%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `63.23% <0%> (ø)` | :arrow_up: |
| [pandas/plotting/\_tools.py](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL190b29scy5weQ==) | `79% <0%> (ø)` | :arrow_up: |
| ... and [6 more](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=footer). Last update [929c66f...ea334c5](https://codecov.io/gh/pandas-dev/pandas/pull/17174?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> this doc-string needs to go in _shared_docs; then you need to add it back to pivot_table itself, which is imported as pd.pivot_table.

Just pushed a commit to to do this.

You mentioned frame.py being too long, which is totally correct.  Would it be helpful to do the same thing with the docstrings for `pivot`, `melt`, `unstack`?
> You mentioned frame.py being too long, which is totally correct. Would it be helpful to do the same thing with the docstrings for pivot, melt, unstack?

melt looks ok to me already, unstack and stack are *only* in frame.py
@jreback You closed by accident?
yes sorry
> hmm I don't like this here the import order shouldn't matter (I know it does, but it shouldn't).

Yah, I very much dislike this part.  My preference would be to put the docstring in `reshape.pivot` and refactor imports in `reshape.pivot` so that `core.frame` can import it at the top.  This spills over into requiring the same import changes in `reshape.concat`, so I figured it would be better to get the go-ahead before trying this (in a separate PR).

> generally we simply import the _shared_docs at the end.

Coming right up
btw, why isn't `_shared_docs` shared between `core.base` and `core.generic`?
> btw, why isn't _shared_docs shared between core.base and core.generic?

its not important for base, rather its sub-class based