Skip to content
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

Array Interface and Categorical internals Refactor #19268

Merged
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
2ef5216
REF: Define extension base classes
TomAugspurger Jan 15, 2018
57e8b0f
Updated for comments
TomAugspurger Jan 18, 2018
01bd42f
Remove metaclasses from PeriodDtype and IntervalDtype
TomAugspurger Jan 18, 2018
ce81706
Fixup form_blocks rebase
TomAugspurger Jan 18, 2018
87a70e3
Restore concat casting cat -> object
TomAugspurger Jan 18, 2018
8c61886
Remove _slice, clarify semantics around __getitem__
TomAugspurger Jan 19, 2018
cb41803
Document and use take.
TomAugspurger Jan 19, 2018
65d5a61
Clarify type, kind, init
TomAugspurger Jan 19, 2018
57c749b
Remove base
TomAugspurger Jan 19, 2018
6736b0f
API: Remove unused __iter__ and get_values
TomAugspurger Jan 21, 2018
e4acb59
API: Implement repr and str
TomAugspurger Jan 21, 2018
0e9337b
Merge remote-tracking branch 'upstream/master' into pandas-array-inte…
TomAugspurger Jan 26, 2018
df68f3b
Remove default value_counts for now
TomAugspurger Jan 26, 2018
2746a43
Fixed merge conflicts
TomAugspurger Jan 27, 2018
34d2b99
Remove implementation of construct_from_string
TomAugspurger Jan 27, 2018
a484d61
Example implementation of take
TomAugspurger Jan 27, 2018
04b2e72
Cleanup ExtensionBlock
TomAugspurger Jan 27, 2018
df0fa12
Merge remote-tracking branch 'upstream/master' into pandas-array-inte…
TomAugspurger Jan 27, 2018
e778053
Pass through ndim
TomAugspurger Jan 27, 2018
d15a722
Use series._values
TomAugspurger Jan 27, 2018
b5f736d
Removed repr, updated take doc
TomAugspurger Jan 27, 2018
240e8f6
Various cleanups
TomAugspurger Jan 28, 2018
f9b0b49
Handle get_values, to_dense, is_view
TomAugspurger Jan 28, 2018
7913186
Docs
TomAugspurger Jan 30, 2018
df18c3b
Remove is_extension, is_bool
TomAugspurger Jan 30, 2018
ab2f045
Sparse formatter
TomAugspurger Jan 30, 2018
520876f
Revert "Sparse formatter"
TomAugspurger Jan 30, 2018
4dfa39c
Unbox SparseSeries
TomAugspurger Jan 30, 2018
e252103
Added test for sparse consolidation
TomAugspurger Jan 30, 2018
7110b2a
Docs
TomAugspurger Jan 30, 2018
c59dca0
Merge remote-tracking branch 'upstream/master' into pandas-array-inte…
TomAugspurger Jan 31, 2018
fc688a5
Moved to errors
TomAugspurger Jan 31, 2018
fbc8466
Handle classmethods, properties
TomAugspurger Jan 31, 2018
030bb19
Use our AbstractMethodError
TomAugspurger Jan 31, 2018
0f4c2d7
Lint
TomAugspurger Jan 31, 2018
f9316e0
Cleanup
TomAugspurger Feb 1, 2018
9c06b13
Move ndim validation to a method.
TomAugspurger Feb 1, 2018
7d2cf9c
Try this
TomAugspurger Feb 1, 2018
afae8ae
Make ExtensionBlock._holder a property
TomAugspurger Feb 1, 2018
cd0997e
Make _holder a property for all
TomAugspurger Feb 1, 2018
1d6eb04
Refactored validate_ndim
TomAugspurger Feb 1, 2018
92aed49
fixup! Refactored validate_ndim
TomAugspurger Feb 1, 2018
34134f2
lint
TomAugspurger Feb 1, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pandas/core/arrays/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
from .base import ExtensionArray # noqa
from .categorical import Categorical # noqa
247 changes: 247 additions & 0 deletions pandas/core/arrays/base.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,247 @@
"""An interface for extending pandas with custom arrays."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd advocate leaving base.py open for (near-)future usage as pandas-internal base and putting the "use this if you want to write your own" file in e.g. extension.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference for base.py since it's a base class for all extension arrays. I don't think that having ExtensionArray in arrays.base precludes having a pandas-internal base there as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be publicly exposed through pd.api.extensions anyway I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding stuff to the public API is waiting on #19304

from pandas.errors import AbstractMethodError

_not_implemented_message = "{} does not implement {}."


class ExtensionArray(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any expected requirements for the constructor __init__?

Copy link
Contributor Author

@TomAugspurger TomAugspurger Jan 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should figure out what those are and document them. At the very least, we expected ExtensionArray(extension_array) to work correctly. I'll look for other assumptions we make. Or that could be pushed to another classmethod.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also expect that ExtensionArray(), with no arguments, works so that subclasses don't have to implement construct_from_string.

Rather than imposing that on subclasses, we could require some kind of .empty alternative constructor.

"""Abstract base class for custom 1-D array types.

pandas will recognize instances of this class as proper arrays
with a custom type and will not attempt to coerce them to objects. They
may be stored directly inside a :class:`DataFrame` or :class:`Series`.

Notes
-----
The interface includes the following abstract methods that must be
implemented by subclasses:

* __getitem__
* __len__
* dtype
* nbytes
* isna
* take
* copy
* _formatting_values
* _concat_same_type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two lines above can be removed here and mentioned in the list below (actually only formatting_values, as concat_same_type is already there)


Some additional methods are required to satisfy pandas' internal, private
block API.

* _concat_same_type
* _can_hold_na

This class does not inherit from 'abc.ABCMeta' for performance reasons.
Methods and properties required by the interface raise
``pandas.errors.AbstractMethodError`` and no ``register`` method is
provided for registering virtual subclasses.

ExtensionArrays are limited to 1 dimension.

They may be backed by none, one, or many NumPy ararys. For example,
``pandas.Categorical`` is an extension array backed by two arrays,
one for codes and one for categories. An array of IPv6 address may
be backed by a NumPy structured array with two fields, one for the
lower 64 bits and one for the upper 64 bits. Or they may be backed
by some other storage type, like Python lists. Pandas makes no
assumptions on how the data are stored, just that it can be converted
to a NumPy array.

Extension arrays should be able to be constructed with instances of
the class, i.e. ``ExtensionArray(extension_array)`` should return
an instance, not error.

Additionally, certain methods and interfaces are required for proper
this array to be properly stored inside a ``DataFrame`` or ``Series``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is repetitive with above (the list of methods that are required), and there is also a typo in "for proper this array to be properly"

"""
# ------------------------------------------------------------------------
# Must be a Sequence
# ------------------------------------------------------------------------
def __getitem__(self, item):
# type (Any) -> Any
"""Select a subset of self.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add Parameters / Returns


Parameters
----------
item : int, slice, or ndarray
* int: The position in 'self' to get.

* slice: A slice object, where 'start', 'stop', and 'step' are
integers or None

* ndarray: A 1-d boolean NumPy ndarray the same length as 'self'

Returns
-------
item : scalar or ExtensionArray

Notes
-----
For scalar ``item``, return a scalar value suitable for the array's
type. This should be an instance of ``self.dtype.type``.

For slice ``key``, return an instance of ``ExtensionArray``, even
if the slice is length 0 or 1.

For a boolean mask, return an instance of ``ExtensionArray``, filtered
to the values where ``item`` is True.
"""
raise AbstractMethodError(self)

def __setitem__(self, key, value):
# type: (Any, Any) -> None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already use AbstractMethodError elsewhere in the code base, use instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setitem must be defined (it certainly does not need to actually set inplace), but since we use this is must appear as a mutable object

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of having it here as NotImplementedError is because an ExtensionArray does not necessarily needs to support setting elements (being mutable), at least that's one possible decision (leave the decision up to the extension author).
The error will then just bubble up to the user if he/she tries to set something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I don't think we can assume that all extension arrays will implement it.

raise NotImplementedError(_not_implemented_message.format(
type(self), '__setitem__')
)

def __len__(self):
"""Length of this array

Returns
-------
length : int
"""
# type: () -> int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document

raise AbstractMethodError(self)

# ------------------------------------------------------------------------
# Required attributes
# ------------------------------------------------------------------------
@property
def dtype(self):
# type: () -> ExtensionDtype
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

examples and document

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would an example even look like here? Defining an ExtensionDtype in the docstring?

"""An instance of 'ExtensionDtype'."""
raise AbstractMethodError(self)

@property
def shape(self):
# type: () -> Tuple[int, ...]
return (len(self),)

@property
def ndim(self):
# type: () -> int
"""Extension Arrays are only allowed to be 1-dimensional."""
return 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be tested on registration of the sub-type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean "registration"? We could override ABC.register, but I don't think there's an (easy) way to validate this if they just subclass ExtensionArray.

If people want to mess with this, that's fine, their stuff just won't work with pandas.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what I mean is that when you register things, we should actually test that the interface is respected. If we had final methods this would not be necessary, but if someone override ndim this is a problem.


@property
def nbytes(self):
# type: () -> int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type comments come before the docstring: http://mypy.readthedocs.io/en/latest/python2.html

"""The number of bytes needed to store this object in memory.

If this is expensive to compute, return an approximate lower bound
on the number of bytes needed.
"""
raise AbstractMethodError(self)

# ------------------------------------------------------------------------
# Additional Methods
# ------------------------------------------------------------------------
def isna(self):
# type: () -> np.ndarray
"""Boolean NumPy array indicating if each value is missing.

This should return a 1-D array the same length as 'self'.
"""
raise AbstractMethodError(self)

# ------------------------------------------------------------------------
# Indexing methods
# ------------------------------------------------------------------------
def take(self, indexer, allow_fill=True, fill_value=None):
# type: (Sequence[int], bool, Optional[Any]) -> ExtensionArray
"""Take elements from an array.

Parameters
----------
indexer : sequence of integers
indices to be taken. -1 is used to indicate values
that are missing.
allow_fill : bool, default True
If False, indexer is assumed to contain no -1 values so no filling
will be done. This short-circuits computation of a mask. Result is
undefined if allow_fill == False and -1 is present in indexer.
fill_value : any, default None
Fill value to replace -1 values with. By default, this uses
the missing value sentinel for this type, ``self._fill_value``.

Notes
-----
This should follow pandas' semantics where -1 indicates missing values.
Positions where indexer is ``-1`` should be filled with the missing
value for this type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's strongly consider exposing a helper function to make this easier to write, or at least an example of what this would look like (we can save this for later). It's not obvious how to write this with NumPy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an example that I hope gets things correct for an extension type backed by a NumPy structured array.

One trouble with this providing a helper function is that we don't know much about how the extension array is actually storing the data. Although, we could rely on the assumption that the underlying storage is convertible to a NumPy array, and proceed from there. Though this would perhaps be sub-optimal for many extension arrays.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For GeometryArray, we have followed the same idea the write take (so it's not necessarily only if you have a structured array, just when you have an array backing up your ExtensionArray)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine as it is a pandas standard.


This is called by ``Series.__getitem__``, ``.loc``, ``iloc``, when the
indexer is a sequence of values.

Examples
--------
Suppose the extension array somehow backed by a NumPy structured array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make this just "NumPy array", as this is not specific to structured arrays

and that the underlying structured array is stored as ``self.data``.
Then ``take`` may be written as

.. code-block:: python

def take(self, indexer, allow_fill=True, fill_value=None):
mask = indexer == -1
result = self.data.take(indexer)
result[mask] = self._fill_value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question here is: should the keyword argument fill_value actually be honored? (like if fill_value is None: fill_value = self._fill_value)
Are there case where pandas will actually pass a certain value?

In any case some clarification would be helpful, also if it is just in the signature for compatibility but may be ignored (maybe in a follow-up).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re fill_value, I based this off Categorical.take. It does assert isna(fill_value), but otherwise ignores it.

I think that since ExtensionArray.take returns an ExtensionArray, most implementations will just ignore fill_value. I'll clarify it in the docs.

return type(self)(result)
"""
raise AbstractMethodError(self)

def copy(self, deep=False):
# type: (bool) -> ExtensionArray
"""Return a copy of the array.

Parameters
----------
deep : bool, default False
Also copy the underlying data backing this array.

Returns
-------
ExtensionArray
"""
raise AbstractMethodError(self)

# ------------------------------------------------------------------------
# Block-related methods
# ------------------------------------------------------------------------
@property
def _fill_value(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to list this in the very top doc-string

# type: () -> Any
"""The missing value for this type, e.g. np.nan"""
return None

def _formatting_values(self):
# type: () -> np.ndarray
# At the moment, this has to be an array since we use result.dtype
"""An array of values to be printed in, e.g. the Series repr"""
raise AbstractMethodError(self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can provide a default implementation of return np.asarray(self) ? (so a densified object array). That is what I do in geopandas, and I suppose would also work for the IPadresses ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I suppose that'll be OK for many implementations.


@classmethod
def _concat_same_type(cls, to_concat):
# type: (Sequence[ExtensionArray]) -> ExtensionArray
"""Concatenate multiple array

Parameters
----------
to_concat : sequence of this type

Returns
-------
ExtensionArray
"""
raise AbstractMethodError(cls)

def _can_hold_na(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to list this in the very top doc-string

# type: () -> bool
"""Whether your array can hold missing values. True by default.

Notes
-----
Setting this to false will optimize some operations like fillna.
"""
return True
18 changes: 17 additions & 1 deletion pandas/core/arrays/categorical.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
from pandas.util._validators import validate_bool_kwarg
from pandas.core.config import get_option

from .base import ExtensionArray


def _cat_compare_op(op):
def f(self, other):
Expand Down Expand Up @@ -148,7 +150,7 @@ def _maybe_to_categorical(array):
"""


class Categorical(PandasObject):
class Categorical(ExtensionArray, PandasObject):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By having our internal arrays inherit from PandasObject, they also get a _constructor method. So we should either make sure this is never used (apart from in methods inside the array itself), or add this to the interface (my preference would be the first)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the methods in PandasObject needs to be ABC in the ExtensionArray

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, I don't think all methods/attributes of PandasObject should be added to the public ExtensionArray (to keep those internal + to not clutter the ExtensionArray API)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I'm consistently testing these changes against

  1. An implementation of IntervalArary: https://github.com/TomAugspurger/pandas/compare/pandas-array-interface-3...TomAugspurger:pandas-array-upstream+interval?expand=1
  2. A branch on pandas-ip: https://github.com/ContinuumIO/pandas-ip/tree/pandas-array-upstream-compat

Neither inherit from PandasObject at the moment, so we're OK.

"""
Represents a categorical variable in classic R / S-plus fashion

Expand Down Expand Up @@ -2130,6 +2132,20 @@ def repeat(self, repeats, *args, **kwargs):
return self._constructor(values=codes, categories=self.categories,
ordered=self.ordered, fastpath=True)

# Implement the ExtensionArray interface
@property
def _can_hold_na(self):
return True

@classmethod
def _concat_same_type(self, to_concat):
from pandas.core.dtypes.concat import _concat_categorical

return _concat_categorical(to_concat)

def _formatting_values(self):
return self

# The Series.cat accessor


Expand Down
16 changes: 2 additions & 14 deletions pandas/core/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@

# compat
from pandas.errors import ( # noqa
PerformanceWarning, UnsupportedFunctionCall, UnsortedIndexError)
PerformanceWarning, UnsupportedFunctionCall, UnsortedIndexError,
AbstractMethodError)

# back-compat of public API
# deprecate these functions
Expand Down Expand Up @@ -88,19 +89,6 @@ class SettingWithCopyWarning(Warning):
pass


class AbstractMethodError(NotImplementedError):
"""Raise this error instead of NotImplementedError for abstract methods
while keeping compatibility with Python 2 and Python 3.
"""

def __init__(self, class_instance):
self.class_instance = class_instance

def __str__(self):
msg = "This method must be defined in the concrete class of {name}"
return (msg.format(name=self.class_instance.__class__.__name__))


def flatten(l):
"""Flatten an arbitrarily nested sequence.

Expand Down
Loading