REF: define concat classmethods in the appropriate places #27727
Conversation
|
|
||
| def _concat_same_dtype(self, to_concat, name): | ||
| """ | ||
| Concatenate to_concat which has the same class. | ||
| """ | ||
| # must be overridden in specific classes | ||
| return _concat._concat_index_asobject(to_concat, name) | ||
| klasses = (ABCDatetimeIndex, ABCTimedeltaIndex, ABCPeriodIndex, ExtensionArray) |
There was a problem hiding this comment.
should there be references to specific Index types in pandas/core/indexes/base.py?
There was a problem hiding this comment.
We've already got a few; in this case Index is serving as less of a base class and more of a ObjectIndex.
There was a problem hiding this comment.
This PR takes the subset of dtypes.concat methods that are a) private and b) equivalent to klass._concat_same_dtype for some klass and moves the implementation to the appropriate class.
dtypes.concat was meant to hold all the concat things so they are in 1 place. Now we have some in Index and some there. Is this pr meant as a precusor to moar consolidation? The reason we moved these in the first place was that there were multiple definitions instead of a single canonical one.
Since these are used exactly once I guess its ok to move them back, but this kind of moves us backwards here IMHO.
I do like the idea of consolidation, but would like to here what where you think this can go first.
Totally on board with the principle here. But
Yes. union_categorical and concat_categorical are left in place because just-do-the-incorrectly-private-stuff seemed like a good stopping point. But these will belong in a location that reflects the fact that they are array functions, possibly with some of the other helper functions in arrays.categorical that are used in not-just-Categorical places. |
|
thanks for the expl @jbrockmendel I think a good goal then would be to remove completely pandas.core.dtypes.concat and move to appropriate places the remaining. |
cc @jorisvandenbossche we briefly discussed at the sprint the idea that
dtype.concatis a weird place to define these functions.This PR takes the subset of
dtypes.concatmethods that are a) private and b) equivalent toklass._concat_same_dtypefor someklassand moves the implementation to the appropriate class.The categorical one is left in place for now because a) it is public and b) it'd be a pretty big move in and of itself.