WIP: Refactor accessors, unify usage, make "recipe" #17042

jbrockmendel · 2017-07-20T19:40:09Z

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
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Make str_normalize as func; rename copy--> copy_doc to avoid stdlib name

Expand some inline if/else blocks

Rename strings classes in tests Update imports in tests

flake8 fixes Update import name

jbrockmendel · 2017-07-20T19:41:20Z

pandas/core/accessors.py

+        def func(self, *args, **kwargs):
+            return self._delegate_method(name, *args, **kwargs)
+
+        if callable(name):


Note to self: you tried this option and decided against it.

jbrockmendel · 2017-07-20T19:42:19Z

pandas/core/accessors.py

+        the class definition.  For things that we cannot keep directly
+        in the class definition, a decorator is more directly tied to
+        the definition than a method call outside the definition.
+


The paragraph above may be helpful for the dev discussion, but probably doesn't belong in the docstring.

jbrockmendel · 2017-07-20T19:42:42Z

pandas/core/accessors.py

+                    func = Delegator.create_delegator_method(name, delegate)
+
+                # Allow for a callable to be passed instead of a name.
+                title = com._get_callable_name(name)


Note to self: you decided against this option.

jbrockmendel · 2017-07-20T19:43:29Z

pandas/core/frame.py

@@ -6006,8 +6005,8 @@ def _put_str(s, space):

 # ----------------------------------------------------------------------
 # Add plotting methods to DataFrame
-DataFrame.plot = base.AccessorProperty(gfx.FramePlotMethods,


Needs rebasing.

jbrockmendel · 2017-07-20T19:45:06Z

pandas/core/indexes/accessors.py

+
+
+DatetimeAccessor = CombinedDatetimelikeProperties
+# Alias to mirror CategoricalAccessor


If we're subclassing PandasDelegate, would it make more sense for these to be named e.g. DatetimeDelegate?

Changing these names over now.

jbrockmendel · 2017-07-20T19:50:56Z

pandas/core/strings.py

+        method = getattr(self.values, name)
+        res = method(*args, **kwargs)
+        # TODO: Should this get wrapped in an index?
+        return res


TODO: even though it isn't explicitly needed, having StringMethods subclass PandasDelegate will clarify its parallels with the other accessors.

Plus actually using _delegate_method could get rid of a lot of boilerplate.

jbrockmendel · 2017-07-20T19:51:47Z

pandas/core/strings.py

+# and the _dir_additions/_dir_deletions won't play nicely with
+# any other class this gets mixed into that *does* implement its own
+# _dir_additions/_dir_deletions.  This should be deprecated.
+class StringAccessorMixin(object):


StringAccessorMixin should be considered for deprecation. See comment above.

this is internal, you can just remove it if not needed

Great; wasn't sure if it should be saved for a separate PR. Doing so requires copy/pasting _accessors, _dir_deletions, and _dir_additions from Series (which I see as a good thing, per #8162 , #17061)

jbrockmendel · 2017-07-20T19:52:42Z

pandas/core/indexes/category.py

+    def __init__(self, *args, **kwargs):
+        # Override to prevent accessors.PandasDelegate.__init__ from executing
+        # This is a kludge.
+        pass


Note to self: fix this.

jbrockmendel · 2017-07-20T19:53:49Z

pandas/core/indexes/accessors.py

@@ -104,6 +106,9 @@ def _delegate_property_get(self, name):
                result = result.astype('int64')
        elif not is_list_like(result):
            return result
+        elif isinstance(result, DataFrame):
+            # e.g. TimedeltaProperties.components
+            return result.set_index(self.index)


This allows _delegate_property_get to correctly handle components.

pep8speaks · 2017-07-20T19:56:14Z

Hello @jbrockmendel! Thanks for updating the PR.

In the file pandas/core/categorical.py, following are the PEP8 issues :

Line 2072:5: E303 too many blank lines (2)

In the file pandas/core/strings.py, following are the PEP8 issues :

Line 1950:1: W391 blank line at end of file

Comment last updated on September 20, 2017 at 04:01 Hours UTC

codecov · 2017-07-20T21:16:10Z

Codecov Report

Merging #17042 into master will decrease coverage by 0.02%.
The diff coverage is 94.41%.

@@            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%)`	⬇️
#single	`40.21% <79.32%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/base.py	`96.56% <ø> (+0.34%)`	⬆️
pandas/core/categorical.py	`95.5% <100%> (+0.03%)`	⬆️
pandas/core/series.py	`94.85% <100%> (-0.06%)`	⬇️
pandas/core/frame.py	`97.75% <100%> (-0.11%)`	⬇️
pandas/core/indexes/category.py	`98.5% <100%> (-0.01%)`	⬇️
pandas/core/accessors.py	`90% <90%> (ø)`
pandas/core/strings.py	`98.07% <94.91%> (-0.41%)`	⬇️
pandas/core/indexes/accessors.py	`88.29% <95.45%> (+0.2%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e58225...953598a. Read the comment docs.

codecov · 2017-07-20T21:16:21Z

Codecov Report

Merging #17042 into master will decrease coverage by 0.19%.
The diff coverage is 97.94%.

@@            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%)`	⬇️
#single	`40.28% <76.92%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/base.py	`96.56% <ø> (+0.54%)`	⬆️
pandas/core/frame.py	`97.65% <100%> (-0.22%)`	⬇️
pandas/core/series.py	`94.89% <100%> (-0.04%)`	⬇️
pandas/core/indexes/base.py	`95.96% <100%> (-0.32%)`	⬇️
pandas/core/strings.py	`97.61% <100%> (-0.85%)`	⬇️
pandas/core/categorical.py	`95.5% <100%> (-0.08%)`	⬇️
pandas/core/indexes/category.py	`98.48% <100%> (-0.07%)`	⬇️
pandas/core/indexes/accessors.py	`88.29% <93.1%> (-0.95%)`	⬇️
pandas/core/accessors.py	`96.36% <96.36%> (ø)`
pandas/io/s3.py	`0% <0%> (-85%)`	⬇️
... and 82 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21a3800...264a7e7. Read the comment docs.

jreback · 2017-07-21T11:30:10Z

pandas/core/accessors.py

+# -*- coding: utf-8 -*-
+"""
+
+An example/recipe for creating a custom accessor.


I would remove all about the custom use of this. This as a refactor is fine, but this would really need a good usecase to actually add a custom delegate. Happy to have this in internals.rst if its necessary.

@shoyer: You commented on this in #14781. Have you found that custom accessors are frequently used by xarray users?

@jreback I can delete it or move it somewhere out of the way. The test_accessors file might make a good place to park the discussion seeing as how it uses the same example anyway.

I'm +1 on the general idea of exposing a way to register custom accessors, though the api in your example seems a little clunky compared to what xarray does? That said, may be better to punt that piece to a follow-up PR.

Agree that docs should live in internals.rts, logical companion to the subclassing docs

example seems a little clunky compared to what xarray does

The examples are for pretty different use cases. The xarray example defines a center attribute in terms of multiple existing [column]s of a [DataFrame]. That's why it doesn't need any vectorization boilerplate.

The example I used vectorizes properties/methods of the elements in a Series. StringMethods or CategoricalAccessor work roughly this way. (CombinedDatetimelikeProperties doesn't need to apply the property/method point-wise because they already exist in the underlying Series/Index.)

That said, if this isn't already obvious, that is a shortcoming of the documentation.

There is some avoidable clunkiness in that we are requiring every accessible attribute to be explicitly listed.

Just pushed commits that address some of these. Took the example out of the module docstring, put a comment in PandasDelegate.__doc__ pointing readers towards the existing examples.

shoyer · 2017-07-22T19:50:39Z

This interface still looks really complicated to me, much more complex than the recipe we have in xarray. Based on the xarray recipe, you could write something like:

@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.,

@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()

jbrockmendel · 2017-07-22T23:46:34Z

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.

shoyer · 2017-07-23T01:49:49Z

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.

jbrockmendel · 2017-07-24T06:50:24Z

If this is a sticking point, I'd argue that there are two refactorings here that are much more important than everything else.

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.
PandasDelegate._add_delegate_accessors took a while to figure out. De-nesting that method could save the next guy a headache.

jbrockmendel · 2017-07-24T16:57:56Z

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?

jreback · 2017-07-24T22:55:58Z

u can certainly try the ML

jreback

generally looks ok. lmk when you are ready ping.

jreback · 2017-07-24T23:28:20Z

pandas/core/accessors.py

+        Delegate : instance of PandasDelegate or subclass
+
+        """
+        raise NotImplementedError(


use AbstractMethodError

jreback · 2017-07-24T23:29:10Z

pandas/core/accessors.py

+                [...]
+
+
+        This replaces the older usage in which following a class definition


you don't need to say what is was, just what it does now.

jreback · 2017-07-24T23:29:39Z

pandas/core/accessors.py

+            typ : 'property' or 'method'
+            overwrite : boolean, default False
+                overwrite the method/property in the target class if it exists
+            """


maybe some asserts to valid types of the accessors and such

jreback · 2017-07-24T23:31:12Z

pandas/core/categorical.py

@@ -2013,7 +2014,20 @@ def repeat(self, repeats, *args, **kwargs):
 # The Series.cat accessor


-class CategoricalAccessor(PandasDelegate, NoNewAttributesMixin):
+@accessors.wrap_delegate_names(delegate=Categorical,


an alternative way to do this is to add in the classes themselves which methods / properties should be considered accessors (either in a class method or via decorators), i did do this for a while, but actually though it simpler here. but can revisit at a later point.

I'm amenable to that. Over in core.indexes.accessors there is a comment above DatetimeAccessor noting an alternative way to define the accessed properties/methods.

The only pattern I actively dislike is CategoricalIndex._add_accessors; pinning attributes to the class from a distance can lead to surprises (like the still-mysterious circular import core.series<-->plotting._core).

jreback · 2017-07-24T23:31:56Z

pandas/core/categorical.py

    def _delegate_method(self, name, *args, **kwargs):
        from pandas import Series
        method = getattr(self.categorical, name)
        res = method(*args, **kwargs)
        if res is not None:
            return Series(res, index=self.index)

+    # TODO: Can we get this from _delegate_property_get?


yes, but its simpler here as it

jreback · 2017-07-24T23:32:45Z

pandas/core/indexes/accessors.py

+
+
+DatetimeAccessor = CombinedDatetimelikeProperties
+# Alias to mirror CategoricalAccessor


jreback · 2017-07-24T23:34:14Z

pandas/core/series.py

-import pandas.core.strings as strings
-from pandas.core.indexes.accessors import (
-    maybe_to_datetimelike, CombinedDatetimelikeProperties)
+


ideally don't move imports around unless they are germane, it makes the diff hard to read

jreback · 2017-07-24T23:35:07Z

pandas/core/strings.py

@@ -1316,6 +1358,27 @@ def str_encode(arr, encoding, errors="strict"):
    return _na_map(f, arr)


+def str_normalize(arr, form):


Nearly all the StringMethods methods follow a pattern of defining str_foo as a module-level function and then wrapping it as the foo method. StringMethods.normalize didn't follow this pattern, so I moved it out. At the time I intended to use _delegate_method to get rid of a bunch of boilerplate in the class (and clarify its relation to other PandasDelegate subclasses), but eventually chose to hold off on that.

So this isn't strictly pertinent to this PR; I'll be happy to revert it.

Would getting rid of the not-strictly-relevant edits here in strings.py grease the wheels on this?

jreback · 2017-07-24T23:36:24Z

pandas/core/strings.py

+# and the _dir_additions/_dir_deletions won't play nicely with
+# any other class this gets mixed into that *does* implement its own
+# _dir_additions/_dir_deletions.  This should be deprecated.
+class StringAccessorMixin(object):


this is internal, you can just remove it if not needed

Implement _dir_additions and _dir_deletions in Index

…to accessory

jbrockmendel · 2017-07-26T16:44:07Z

Does the CircleCI timeout require action?

chris-b1 · 2017-07-26T16:48:55Z

I restarted it; think it's been a bit flaky

gfyoung · 2017-07-26T16:52:05Z

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 😢

jreback · 2017-08-08T23:50:51Z

you can rebase or close this. prob easier to do step by step changes.

jbrockmendel · 2017-08-09T16:42:52Z

Do you mind letting this stay open until #17117 is resolved? I'll have to rebase again after that anyway.

jbrockmendel added 6 commits July 18, 2017 22:25

Move PandasDelegate and AccessorProperty; update imports

b77e103

Move apply _shared_docs to functions and attach to methods with copy

dbc149d

Make str_normalize as func; rename copy--> copy_doc to avoid stdlib name

Implement _make_accessor as classmethod on StringMethods

3c77d94

Expand some inline if/else blocks

Add example/recipe

19f7ff6

Test to go along with example/recipe

d152421

Rename strings classes in tests Update imports in tests

Transition to _make_accessor

101e7e5

flake8 fixes Update import name

jbrockmendel commented Jul 20, 2017

View reviewed changes

Merge branch 'master' into accessory

774a35d

Merge branch 'master' into accessory

ccec595

gfyoung added API Design Clean Internals Related to non-user accessible pandas implementation labels Jul 20, 2017

jbrockmendel added 2 commits July 20, 2017 14:13

Remove unused import that was causing a lint error

74e4539

merge pulled

953598a

jreback reviewed Jul 21, 2017

View reviewed changes

jbrockmendel added 3 commits July 22, 2017 09:17

Wrap long line

22d4892

Refactor tests and documentation

014fae0

Typos, flake8 fixes, rearrange comments

dd8315c

jbrockmendel force-pushed the accessory branch from e145ae8 to dd8315c Compare July 22, 2017 17:43

Simplify categorical make_accessor args

74a237b

jbrockmendel mentioned this pull request Jul 24, 2017

Index/Series Convergence #17061

Closed

jreback reviewed Jul 24, 2017

View reviewed changes

jbrockmendel added 6 commits July 25, 2017 08:33

Rename PandasDelegate subclasses FooDelegate

c931d4b

Revert import rearrangement; update names FooDelegate

6c771b4

Deprecate StringAccessorMixin

d3a4460

Implement _dir_additions and _dir_deletions in Index

Merge branch 'master' into accessory

48f3b4d

lint fixes

73a0633

Merge branch 'accessory' of https://github.com/jbrockmendel/pandas in…

aa793ad

…to accessory

This was referenced Jul 29, 2017

Unify Index._dir_* with Series implementation #17117

Merged

DISC: add accessor attributes to Index for consistency with Series #17134

Open

Implement _make_accessor classmethod for PandasDelegate #17166

Merged

Merge branch 'master' into accessory

264a7e7

This was referenced Sep 23, 2017

cut/paste AccessorProperty and PandasDelegate to core.accessor #17651

Merged

implement Delegator class #17662

Closed

jbrockmendel closed this Sep 28, 2017

jbrockmendel deleted the accessory branch December 8, 2017 19:41

jbrockmendel mentioned this pull request Dec 18, 2017

ENH: Added public accessor registrar #18827

Merged



		DatetimeAccessor = CombinedDatetimelikeProperties
		# Alias to mirror CategoricalAccessor

		[...]


		This replaces the older usage in which following a class definition

		@@ -1316,6 +1358,27 @@ def str_encode(arr, encoding, errors="strict"):
		return _na_map(f, arr)


		def str_normalize(arr, form):

WIP: Refactor accessors, unify usage, make "recipe" #17042

WIP: Refactor accessors, unify usage, make "recipe" #17042

Conversation

jbrockmendel commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jul 20, 2017 • edited Loading

Comment last updated on September 20, 2017 at 04:01 Hours UTC

codecov bot commented Jul 20, 2017

Codecov Report

codecov bot commented Jul 20, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

shoyer commented Jul 22, 2017

jbrockmendel commented Jul 22, 2017

shoyer commented Jul 23, 2017

jbrockmendel commented Jul 24, 2017

jbrockmendel commented Jul 24, 2017

jreback commented Jul 24, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jul 26, 2017

chris-b1 commented Jul 26, 2017

gfyoung commented Jul 26, 2017

jreback commented Aug 8, 2017

jbrockmendel commented Aug 9, 2017

jbrockmendel Jul 20, 2017 •

edited

Loading

pep8speaks commented Jul 20, 2017 •

edited

Loading

codecov bot commented Jul 20, 2017 •

edited

Loading

jbrockmendel Jul 22, 2017 •

edited

Loading