REF: Implement NDArrayBackedExtensionArray #33660
Conversation
pandas/core/arrays/datetimelike.py
Outdated
| # NB: A bunch of Interval tests fail if we use ._data | ||
| return self.asi8 | ||
|
|
||
| def _from_backing_data(self, arr: np.ndarray): |
| ------ | ||
| ValueError | ||
| """ | ||
| raise AbstractMethodError(self) |
There was a problem hiding this comment.
BaseMaskedArray is in core/arrays/masked.py. If the purpose of this is to create a common base class for another subset of extensionarrays, can we make the module names for the base classes more consistent.
We could also restructure the extension array tests with a similar hierarchy, so that could influence the naming.
There was a problem hiding this comment.
I'm open to other naming ideas; what do you have in mind?
There was a problem hiding this comment.
we already have core/arrays/numpy_ for PandasArray, so the simple numpy name is taken. The difference though is a PandasArray can be instantiated (It is also possible to instantiate a BaseMaskedArray directly) whereas NDArrayBackedExtensionArray is abstract. maybe we need a subdirectory of core/arrays for the base classes.
There was a problem hiding this comment.
could reasonably put the mixin in arrays.numpy_
| @@ -1780,85 +1781,17 @@ def fillna(self, value=None, method=None, limit=None): | |||
|
|
|||
| return self._constructor(codes, dtype=self.dtype, fastpath=True) | |||
|
|
|||
| def take(self, indexer, allow_fill: bool = False, fill_value=None): | |||
| """ | |||
There was a problem hiding this comment.
Probably want to restore this docstring.
There was a problem hiding this comment.
restored with TypeError->ValueError and whatsnew
|
|
||
| def _from_backing_data(self, arr: np.ndarray): | ||
| # Note: we do not retain `freq` | ||
| return type(self)(arr, dtype=self.dtype) # type: ignore |
There was a problem hiding this comment.
unlike Frame and Series, EAs do not have the concept of _constructor. could this be useful?
the exception is Categorical which uses _constructor. so maybe either extend to all EAs or can we remove from Categorical for consistency.
There was a problem hiding this comment.
IIUC Categorical has _constructor because it subclasses PandasObject.
_from_backing_data is specifically about round-tripping or commutativity; not really a good fit for _constructor.
| return self._codes | ||
|
|
||
| def _from_backing_data(self, arr: np.ndarray): | ||
| return self._constructor(arr, dtype=self.dtype, fastpath=True) |
There was a problem hiding this comment.
see other comment re _constructor.
There was a problem hiding this comment.
IIUC Categorical has _constructor because it subclasses PandasObject.
_from_backing_datais specifically about round-tripping or commutativity; not really a good fit for _constructor.
in PandasObject _constructor is type(self), in Categorical, _constructor is Categorical (i.e type(self)). so I think it may worth investigating whether we can remove _constructor from Categorical and use type(self) here to be consistent with datetimelike._from_backing_data.
can u add return types for _from_backing_data, both here and in datetimelike
|
|
||
| _ndarray: np.ndarray | ||
|
|
||
| def _from_backing_data(self: _T, arr: np.ndarray) -> _T: |
There was a problem hiding this comment.
is typing self needed?
There was a problem hiding this comment.
my understanding is that is how we indicate that the return type is "same type as self", but id defer to @simonjayhawkins on this
There was a problem hiding this comment.
yes. T is a typevar, i.e. can take on different types (could be subtypes of a type or union of types) so adding to self binds the typevar and the return type is the same as self.
This can sometimes cause problems in Mixins, but here the 'Mixin' is IMO an abstract base class.
Many EAs are thin wrappers around np.ndarray. We can both de-duplicate a bunch of code and make things easier on downstream authors by implementing
NDArrayBackedExtensionArrayas a base class for such EAs.This PR only implements
NDArrayBackedExtensionArray.take, but there is quite a bit more that can be shared in follow-ups:__getitem__,__setitem__, reductions, ...The only change in logic this PR makes is to make
Categorical.takeraise a ValueError instead of aTypeError, matching DTA/TDA/PA.