WIP: Refactor accessors, unify usage, make "recipe"
State: closed

xref #14781, #16890

There are currently accessors for `dt`, `str`, `cat`, and `plot`.  These are each constructed in different ways and using slightly different naming conventions, turning a fairly simple underlying logic into a bit of a maze.  This goals for this PR are to:

- Document a recipe for users to create custom accessors.  This now boils down to "subclass PandasDelegate".
- Unify the ways in which these accessors are constructed, including clarifying naming conventions.
- Keep never-directly-used methods like `_make_dt_accessor`, `_make_cat_accessor` out of the `Series` namespace.
- Avoid injecting names into existing namespaces.


The biggest simplification (and the main change in logic) is in where the `_make_foo_accessor` is defined.  Under the status quo, this looks roughly like:

```
    def _make_foo_acessor(self):
        [validation that self is foo-like]
        return FooDelegate(self)

    foo = AccessorProperty(FooDelegate, _make_foo_acessor)
```

This PR changes that, so that `_make_foo_accessor` is now a classmethod `_make_accessor` of `FooDelegate`.  Then in `Series` all we need is `foo = AccessorProperty(FooDelegate)`.


At the moment the documentation/recipe is in the form of an example in the module docstring to `core.accessors`.  Not sure where that belongs.  I'm not wild about the going back-and-forth between code that is copy/paste-able and ">>>" interludes.  `test_accessors` is a bare-bones copy of that example with some assertions.


 - [ ] closes #xxxx
 - [ ] tests added / passed
 - [x] passes ``git diff upstream/master -u -- "*.py" | flake8 --diff``
 - [ ] whatsnew entry


Labels:
name: API Design
name: Internals
description: Related to non-user accessible pandas implementation
name: Clean

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





 - In the file [`pandas/core/categorical.py`](https://github.com/pandas-dev/pandas/blob/264a7e77b26b057743073c38d16f68f826e3eafc/pandas/core/categorical.py), following are the PEP8 issues :

> [Line 2072:5](https://github.com/pandas-dev/pandas/blob/264a7e77b26b057743073c38d16f68f826e3eafc/pandas/core/categorical.py#L2072): [E303](https://duckduckgo.com/?q=pep8%20E303) too many blank lines (2)











 - In the file [`pandas/core/strings.py`](https://github.com/pandas-dev/pandas/blob/264a7e77b26b057743073c38d16f68f826e3eafc/pandas/core/strings.py), following are the PEP8 issues :

> [Line 1950:1](https://github.com/pandas-dev/pandas/blob/264a7e77b26b057743073c38d16f68f826e3eafc/pandas/core/strings.py#L1950): [W391](https://duckduckgo.com/?q=pep8%20W391) blank line at end of file













##### Comment last updated on September 20, 2017 at 04:01 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=h1) Report
> Merging [#17042](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/8e582254e3bbdd717ec8193364420913a7fc786d?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `94.41%`.

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

```diff
@@            Coverage Diff             @@
##           master   #17042      +/-   ##
==========================================
- Coverage   91.02%   90.99%   -0.03%     
==========================================
  Files         161      162       +1     
  Lines       49308    49344      +36     
==========================================
+ Hits        44883    44903      +20     
- Misses       4425     4441      +16
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.77% <94.41%> (-0.01%)` | :arrow_down: |
| #single | `40.21% <79.32%> (-0.05%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYmFzZS5weQ==) | `96.56% <ø> (+0.34%)` | :arrow_up: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `95.5% <100%> (+0.03%)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.85% <100%> (-0.06%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <100%> (-0.11%)` | :arrow_down: |
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `98.5% <100%> (-0.01%)` | :arrow_down: |
| [pandas/core/accessors.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWNjZXNzb3JzLnB5) | `90% <90%> (ø)` | |
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `98.07% <94.91%> (-0.41%)` | :arrow_down: |
| [pandas/core/indexes/accessors.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9hY2Nlc3NvcnMucHk=) | `88.29% <95.45%> (+0.2%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| ... and [1 more](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17042?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/17042?src=pr&el=footer). Last update [8e58225...953598a](https://codecov.io/gh/pandas-dev/pandas/pull/17042?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/17042?src=pr&el=h1) Report
> Merging [#17042](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/21a38008e3cab7a0459cce4fab4ace11379c3148?src=pr&el=desc) will **decrease** coverage by `0.19%`.
> The diff coverage is `97.94%`.

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

```diff
@@            Coverage Diff            @@
##           master   #17042     +/-   ##
=========================================
- Coverage   91.19%      91%   -0.2%     
=========================================
  Files         163      162      -1     
  Lines       49626    49430    -196     
=========================================
- Hits        45258    44984    -274     
- Misses       4368     4446     +78
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.78% <97.94%> (-0.19%)` | :arrow_down: |
| #single | `40.28% <76.92%> (+0.02%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYmFzZS5weQ==) | `96.56% <ø> (+0.54%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.65% <100%> (-0.22%)` | :arrow_down: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.89% <100%> (-0.04%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `95.96% <100%> (-0.32%)` | :arrow_down: |
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `97.61% <100%> (-0.85%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `95.5% <100%> (-0.08%)` | :arrow_down: |
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `98.48% <100%> (-0.07%)` | :arrow_down: |
| [pandas/core/indexes/accessors.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9hY2Nlc3NvcnMucHk=) | `88.29% <93.1%> (-0.95%)` | :arrow_down: |
| [pandas/core/accessors.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWNjZXNzb3JzLnB5) | `96.36% <96.36%> (ø)` | |
| [pandas/io/s3.py](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree#diff-cGFuZGFzL2lvL3MzLnB5) | `0% <0%> (-85%)` | :arrow_down: |
| ... and [82 more](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17042?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/17042?src=pr&el=footer). Last update [21a3800...264a7e7](https://codecov.io/gh/pandas-dev/pandas/pull/17042?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

This interface still looks really complicated to me, much more complex than [the recipe we have in xarray](http://xarray.pydata.org/en/stable/internals.html#extending-xarray). Based on the xarray recipe, you could write something like:
```python
@pd.register_series_accessor('my_accessor')
class MyAccessor(object):
    def __init__(self, series):
        ...
```
and voila, `series.my_accessor` returns a `MyAccessor` object.

In your case, library writers need to define lots of special methods like `_make_accessor`, `_delegate_property_get`, etc., not to mention the decorator that requires explicitly enumerating every method.

Why is all this necessary? This sort of complexity is OK internally, but not desirable for user facing API.

One reason I can think of is that we don't even define accessors if they aren't valid (e.g., `.cat` only exists on Series with a categorical dtype). But even here, I think this could require one extra method at most, e.g.,
```python
@pd.register_series_accessor('cat')
class CategoricalAccessor(object):
    def __init__(self, series):
        ...
    @classmethod
    def _should_exist(cls, series):
        # it might also work to just raise an error in __init__
        # in case where the accessor is invalid
        return series.dtype.is_categorical()
```
> Why is all this necessary? This sort of complexity is OK internally

@shoyer The first and most accurate answer is that it isn't, but there was a limit to how much legacy logic I wanted to change in my first non-trivial PR here.

Second, be sure that you are comparing analogous functions.  `xr.register_dataset_accessor('geo')(GeoAccessor)` corresponds roughly to `str = accessors.AccessorProperty(StringAccessor)`.

Using `_make_accessor` instead of just putting the validation in `__init__` is there for the branching cases in `pd.core.indexes.accessors`.  I mainly focused on getting `_make_dt_accessor` and `_make_cat_accessor` out of the `Series` namespace.  Now that I think about it, checking for `_make_accessor` and then falling back to `__init__` might be simpler.

I haven't looked through the history, but I expect that explicitly listing the delegated names was motivated by the desire to surface some properties/methods `DatetimeIndex` without inheriting all of it.
> The first and most accurate answer is that it isn't, but there was a limit to how much legacy logic I wanted to change in my first non-trivial PR here.

This makes sense. I'll leave it up to others to judge if this internal clean-up is worth it, but let's refrain from adding any new public API until we have that cleaner interface.
If this is a sticking point, I'd argue that there are two refactorings here that are much more important than everything else.

1) `AccessorProperty.__init__` currently takes arguments `accessor_cls` and `construct_accesor`.  That constructor can often just be `accessor_cls.__init__`, but should *always* be method of the `accessor_cls`.  The `Series` (and `StringAccessorMixin`) namespace is definitely not where these belong.

2) `PandasDelegate._add_delegate_accessors` took a while to figure out.  De-nesting that method could save the next guy a headache.

A note transplanted from #17061: `StringAccessorMixin` can/should be deprecated.  Doing so requires only a small fraction of this PR.

Then `Index` can have `_accessor`, `_dir_deletions`, and `dir_additions` copy/pasted from Series (or both could inherit from something more generic).

@jreback Would you prefer 17061-style discussion go somewhere else like the mailing list?

u can certainly try the ML

Does the CircleCI timeout require action?
I restarted it; think it's been a bit flaky
> I restarted it; think it's been a bit flaky

Yeah, the CI services can sometimes be annoying like.  Travis I've found is generally the least "disobedient" (for lack of a better term), but it has its own share of problems :cry: 
you can rebase or close this. prob easier to do step by step changes.
Do you mind letting this stay open until #17117 is resolved?  I'll have to rebase again after that anyway.