-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
Changes from 5 commits
2ef5216
57e8b0f
01bd42f
ce81706
87a70e3
8c61886
cb41803
65d5a61
57c749b
6736b0f
e4acb59
0e9337b
df68f3b
2746a43
34d2b99
a484d61
04b2e72
df0fa12
e778053
d15a722
b5f736d
240e8f6
f9b0b49
7913186
df18c3b
ab2f045
520876f
4dfa39c
e252103
7110b2a
c59dca0
fc688a5
fbc8466
030bb19
0f4c2d7
f9316e0
9c06b13
7d2cf9c
afae8ae
cd0997e
1d6eb04
92aed49
34134f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
from .base import ExtensionArray # noqa | ||
from .categorical import Categorical # noqa |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
"""An interface for extending pandas with custom arrays.""" | ||
import abc | ||
|
||
import numpy as np | ||
|
||
from pandas.compat import add_metaclass | ||
|
||
|
||
_not_implemented_message = "{} does not implement {}." | ||
|
||
|
||
@add_metaclass(abc.ABCMeta) | ||
class ExtensionArray(object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any expected requirements for the constructor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also expect that Rather than imposing that on subclasses, we could require some kind of |
||
"""Abstract base class for custom array types | ||
|
||
Notes | ||
----- | ||
pandas will recognize instances of this class as proper arrays | ||
with a custom type and will not attempt to coerce them to objects. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needs much more detail about what is expected here (or at least some examples), e.g. 1-D array-like or whatever There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can leave general docs until this is actually working (the above sentence is at the moment not yet true), which will only be in follow-up PRs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a bit about 1-D and some high-level examples. |
||
|
||
**Restrictions on your class constructor** | ||
|
||
* Your class should be able to be constructed with no arguments, | ||
i.e. ``ExtensionArray()`` returns an instance. | ||
TODO: See comment in ``ExtensionDtype.construct_from_string`` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exactly is I don't understand the comment about Also, in my opinion is somewhat poor API design for an empty constructor to automatically correspond to a certain result. I would rather have an explicit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignore my comments about |
||
* Your class should be able to be constructed with instances of | ||
our class, i.e. ``ExtensionArray(extension_array)`` should returns | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should "our class" be "your class" ? Or should it be able to handle any ExtensionArray subclass (the first would be better IMO) |
||
an instance. | ||
""" | ||
# ------------------------------------------------------------------------ | ||
# Must be a Sequence | ||
# ------------------------------------------------------------------------ | ||
@abc.abstractmethod | ||
def __getitem__(self, item): | ||
# type (Any) -> Any | ||
"""Select a subset of self | ||
|
||
Notes | ||
----- | ||
As a sequence, __getitem__ should expect integer or slice ``key``. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also boolean mask? |
||
|
||
For slice ``key``, you should return an instance of yourself, even | ||
if the slice is length 0 or 1. | ||
|
||
For scalar ``key``, you may return a scalar suitable for your type. | ||
The scalar need not be an instance or subclass of your array type. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is "need not be" enough? (compared to "should not be") There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll clarify this to say
My earlier phrasing was to explain that the return value for scalars needn't be the type of item that's actually stored in your array. E.g. for my IPAddress example, the array holds two uint64s, but a scalar slice returns an |
||
""" | ||
|
||
def __setitem__(self, key, value): | ||
# type: (Any, Any) -> None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already use AbstractMethodError elsewhere in the code base, use instead There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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__') | ||
) | ||
|
||
@abc.abstractmethod | ||
def __iter__(self): | ||
# type: () -> Iterator | ||
pass | ||
|
||
@abc.abstractmethod | ||
def __len__(self): | ||
# type: () -> int | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. document |
||
pass | ||
|
||
# ------------------------------------------------------------------------ | ||
# Required attributes | ||
# ------------------------------------------------------------------------ | ||
@property | ||
def base(self): | ||
"""The base array I am a view of. None by default.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you give an example here? Perhaps it would also help to explain how is this used by pandas? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's used in If that's correct, then I think we're OK with saying this purely for compatibility with NumPy arrays, and has no effect. I've currently defined There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that is the case, I would remove this for now (we can always later extend the interface if it turns out to be needed for something). However, was just wondering: your ExtensionArray could be a view on another ExtensionArray (eg by slicing). Is this something we need to consider? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would also remove this. NumPy doesn't always maintain this properly, so it can't actually be essential. |
||
|
||
@property | ||
@abc.abstractmethod | ||
def dtype(self): | ||
# type: () -> ExtensionDtype | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. examples and document There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would an example even look like here? Defining an |
||
"""An instance of 'ExtensionDtype'.""" | ||
|
||
@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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be tested on registration of the sub-type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean "registration"? We could override If people want to mess with this, that's fine, their stuff just won't work with pandas. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
@abc.abstractmethod | ||
def nbytes(self): | ||
# type: () -> int | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What should a user do if this is expensive or otherwise difficult to calculate properly? For example, if it's a numpy array with We should probably note that it's OK for this to be an approximate answer (a lower bound) on the number of required bytes, and consider adding another method for the benefit of |
||
|
||
# ------------------------------------------------------------------------ | ||
# Additional Methods | ||
# ------------------------------------------------------------------------ | ||
@abc.abstractmethod | ||
def isna(self): | ||
# type: () -> np.ndarray | ||
"""Boolean NumPy array indicating if each value is missing.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same length as self |
||
|
||
# ------------------------------------------------------------------------ | ||
# Indexing methods | ||
# ------------------------------------------------------------------------ | ||
@abc.abstractmethod | ||
def take(self, indexer, allow_fill=True, fill_value=None): | ||
# type: (Sequence, bool, Optional[Any]) -> ExtensionArray | ||
"""For slicing""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should clarify what valid values of |
||
|
||
@abc.abstractmethod | ||
def copy(self, deep=False): | ||
# type: (bool) -> ExtensionArray | ||
"""Return a copy of the array.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. document. |
||
|
||
# ------------------------------------------------------------------------ | ||
# Block-related methods | ||
# ------------------------------------------------------------------------ | ||
@property | ||
def _fill_value(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
@abc.abstractmethod | ||
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""" | ||
|
||
@classmethod | ||
@abc.abstractmethod | ||
def _concat_same_type(cls, to_concat): | ||
# type: (Sequence[ExtensionArray]) -> ExtensionArray | ||
"""Concatenate multiple array | ||
|
||
Parameters | ||
---------- | ||
to_concat : sequence of this type | ||
|
||
Returns | ||
------- | ||
ExtensionArray | ||
""" | ||
|
||
@abc.abstractmethod | ||
def get_values(self): | ||
# type: () -> np.ndarray | ||
"""A NumPy array representing your data. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's clarify:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not used by
So let's hold off on discussing this until I can understand it better. We could potentially remove it. And if extension arrays are iterable, then we could just as well use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is also used for eg concatting that ends up in object dtype, eg in case of concatting different types (a rather corner case). I think in general this is the "fallback numpy array" in case we need one and we need to be able to infer what to do with it (so typically the object array of custom scalars). It could be an option to use this to enable (or enable good error message) operations. For example that those values are used when the user calls for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep. That sounds right.
Agreed on both accounts, though I think getting reductions, cumulative ops, and numeric ops to all work properly on extension arrays will require some careful thought an quite a bit of work. At the moment, I'm hoping to just avoid breaking things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #19318 for notes about |
||
""" | ||
|
||
def _can_hold_na(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
def _slice(self, slicer): | ||
# type: (Union[tuple, Sequence, int]) -> 'ExtensionArray' | ||
"""Return a new array sliced by `slicer`. | ||
|
||
Parameters | ||
---------- | ||
slicer : slice or np.ndarray | ||
If an array, it should just be a boolean mask | ||
|
||
Returns | ||
------- | ||
array : ExtensionArray | ||
Should return an ExtensionArray, even if ``self[slicer]`` | ||
would return a scalar. | ||
""" | ||
return type(self)(self[slicer]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This default implementation is likely to fail for some obvious implementations. Perhaps we can have a constructor method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me see if I can verify that this is always called with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I would try to get rid of this if possible, and just ask that |
||
|
||
def value_counts(self, dropna=True): | ||
"""Optional method for computing the histogram of the counts. | ||
|
||
Parameters | ||
---------- | ||
dropna : bool, default True | ||
whether to exclude missing values from the computation | ||
|
||
Returns | ||
------- | ||
counts : Series | ||
""" | ||
from pandas.core.algorithms import value_counts | ||
mask = ~np.asarray(self.isna()) | ||
values = self[mask] # XXX: this imposes boolean indexing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should have a dedicated method for boolean indexing or document this as part of the expected interface for |
||
return value_counts(np.asarray(values), dropna=dropna) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,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): | ||
|
@@ -149,7 +151,7 @@ def _maybe_to_categorical(array): | |
""" | ||
|
||
|
||
class Categorical(PandasObject): | ||
class Categorical(ExtensionArray, PandasObject): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By having our internal arrays inherit from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, the methods in PandasObject needs to be ABC in the ExtensionArray There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, I'm consistently testing these changes against
Neither inherit from |
||
""" | ||
Represents a categorical variable in classic R / S-plus fashion | ||
|
||
|
@@ -2131,6 +2133,21 @@ def repeat(self, repeats, *args, **kwargs): | |
return self._constructor(values=codes, categories=self.categories, | ||
ordered=self.ordered, fastpath=True) | ||
|
||
# Interface things | ||
# can_hold_na, concat_same_type, formatting_values | ||
@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 | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
"""Extend pandas with custom array types""" | ||
import abc | ||
|
||
from pandas.compat import add_metaclass | ||
|
||
|
||
@add_metaclass(abc.ABCMeta) | ||
class ExtensionDtype(object): | ||
"""A custom data type for your array. | ||
""" | ||
@property | ||
@abc.abstractmethod | ||
def type(self): | ||
# type: () -> type | ||
"""The scalar type for your array, e.g. ``int`` or ``object``.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not encourage using |
||
|
||
@property | ||
def kind(self): | ||
# type () -> str | ||
"""A character code (one of 'biufcmMOSUV'), default 'O' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should clarify how it's used. How is this useful? Perhaps "This should match |
||
|
||
This should match the NumPy dtype used when your array is | ||
converted to an ndarray, which is probably 'O' for object. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clarify: (if your data type does not corespond to a built-in numpy dtype) |
||
|
||
See Also | ||
-------- | ||
numpy.dtype.kind | ||
""" | ||
return 'O' | ||
|
||
@property | ||
@abc.abstractmethod | ||
def name(self): | ||
# type: () -> str | ||
"""A string identifying the data type. | ||
|
||
Will be used for display in, e.g. ``Series.dtype`` | ||
""" | ||
|
||
@property | ||
def names(self): | ||
# type: () -> Optional[List[str]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Numpy has this for structured dtypes. Not sure if this is needed in the interface though (I don't think we will use it internally, on the other hand this is compatible with numpy) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use it in elif isinstance(data, (np.ndarray, Series, Index)):
if data.dtype.names: I could of course modify that, but perhaps as a followup. For now having a non-abstract names property seems OK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add an example here (or maybe more expl) |
||
"""Ordered list of field names, or None if there are no fields.""" | ||
return None | ||
|
||
@classmethod | ||
def construct_from_string(cls, string): | ||
"""Attempt to construct this type from a string. | ||
|
||
Parameters | ||
---------- | ||
string : str | ||
|
||
Returns | ||
------- | ||
self : instance of 'cls' | ||
|
||
Raises | ||
------ | ||
TypeError | ||
|
||
Notes | ||
----- | ||
The default implementation checks if 'string' matches your | ||
type's name. If so, it calls your class with no arguments. | ||
""" | ||
if string == cls.name: | ||
# XXX: Better to mandate a ``.from_empty`` classmethod | ||
# rather than imposing this on the constructor? | ||
return cls() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the very least, this requirement for the constructor should be documented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still need to document this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps it's best to just remove the default implementation and make it abstract? I'll document current implementation as a possible default. |
||
else: | ||
raise TypeError("Cannot construct a '{}' from " | ||
"'{}'".format(cls, string)) | ||
|
||
@classmethod | ||
def is_dtype(cls, dtype): | ||
"""Check if we match 'dtype' | ||
|
||
Parameters | ||
---------- | ||
dtype : str or dtype | ||
|
||
Returns | ||
------- | ||
is_dtype : bool | ||
|
||
Notes | ||
----- | ||
The default implementation is True if | ||
|
||
1. 'dtype' is a string that returns true for | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "returns true" -> "does not raise" ? |
||
``cls.construct_from_string`` | ||
2. 'dtype' is ``cls`` or a subclass of ``cls``. | ||
""" | ||
if isinstance(dtype, str): | ||
try: | ||
return isinstance(cls.construct_from_string(dtype), cls) | ||
except TypeError: | ||
return False | ||
else: | ||
return issubclass(dtype, cls) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1685,6 +1685,38 @@ def is_extension_type(arr): | |
return False | ||
|
||
|
||
def is_extension_array_dtype(arr_or_dtype): | ||
"""Check if an object is a pandas extension array type | ||
|
||
Parameters | ||
---------- | ||
arr_or_dtype : object | ||
|
||
Returns | ||
------- | ||
bool | ||
|
||
Notes | ||
----- | ||
This checks whether an object implements the pandas extension | ||
array interface. In pandas, this includes: | ||
|
||
* Categorical | ||
* PeriodArray | ||
* IntervalArray | ||
* SparseArray | ||
|
||
Third-party libraries may implement arrays or types satisfying | ||
this interface as well. | ||
""" | ||
from pandas.core.arrays import ExtensionArray | ||
|
||
# we want to unpack series, anything else? | ||
if isinstance(arr_or_dtype, ABCSeries): | ||
arr_or_dtype = arr_or_dtype.values | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will only work if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use |
||
return isinstance(arr_or_dtype, (ExtensionDtype, ExtensionArray)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you pass this function an ExtensionArray subclass, you will get that here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The result is just True or False. The argument can be either an array or dtype. I'm not sure that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not following what we do in all other cases that's my point. pls use _get_dtype_or_type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That won't work unfortunately. In [1]: import pandas as pd; import pandas_ip as ip
In [2]: arr = ip.IPAddress([1, 2, 3])
In [3]: pd.core.dtypes.common._get_dtype_type(arr)
Out[3]: pandas_ip.block.IPv4v6Base
In [4]: isinstance(arr[0], ip.block.IPv4v6Base)
Out[4]: True
In [5]: issubclass(ip.block.IPv4v6Base, pd.core.dtypes.base.ExtensionDtype)
Out[5]: False
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But that would make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What's the issue with the function as defined? I need a way to tell if an array or dtype is an ExtensionArray or ExtensionDtype. Someday, when |
||
|
||
|
||
def is_complex_dtype(arr_or_dtype): | ||
""" | ||
Check whether the provided array or dtype is of a complex dtype. | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 havingExtensionArray
inarrays.base
precludes having a pandas-internal base there as well.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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