ENH: concat of nullable int + bool preserves int dtype #34985
Conversation
| if not all( | ||
| isinstance(t, _IntegerDtype) | ||
| or (isinstance(t, np.dtype) and np.issubdtype(t, np.integer)) | ||
| isinstance(t, BaseMaskedDtype) |
There was a problem hiding this comment.
I worry a bit about this. I could easily see other arrays using the BaseMasked stuff (e.g. StringArray) that shouldn't necessarily be considered "integer-like" for this concat.
So I'd be more comfortable with (_IntegerDtype, BooleanDtype) even though those are synonymous today.
There was a problem hiding this comment.
agree with @TomAugspurger here
There was a problem hiding this comment.
The check for BaseMaskedDtype ensures the dtype has a numpy_dtype, and below we still do a np.find_common_type on the result. So assuming you have int + string, numpy will return object dtype for that, in which case we still return None from this function (which is equivalent as making the check here more strict and returning None here).
So even when we make StringArray a masked array, this method should already work as expected.
And doing it this way, I don't have to add FloatingDtype to the list of (_IntegerDtype, BooleanDtype) in the floating PR.
There was a problem hiding this comment.
So even when we make StringArray a masked array, this method should already work as expected.
Good to know.
I don't have to add FloatingDtype to the list of (_IntegerDtype, BooleanDtype) in the floating PR.
Will we want to return None here for float? Or I suppose the find_common_type stuff will handle that as well, just like string?
There was a problem hiding this comment.
@jreback does #34985 (comment) make sense?
There was a problem hiding this comment.
make sense, though still feel that there is no downside to being explict about listing the 2 dtypes directy; its more obvious. Agreed that if in the future we add more convertable to integer dtypes they won't automatically be added, but i think that is of lesser benefit that better readability here (i mean you could add a comment, but i think listing the classes is better)
There was a problem hiding this comment.
I think how it is written now also gives a good message: all BaseMaskedDtype subclasses are supported by this method (which is the case, even though some might return None from the function later on)
Will we want to return None here for float? Or I suppose the find_common_type stuff will handle that as well, just like string?
Yes, right now we would still return None for float (only if the common numpy dtype is an integer dtype, an EA dtype is returned). Once we have FloatingArray, we would add an additional check for the case that the common numpy dtype is a float dtype, and then return an EA floating dtype.
|
Thanks. Does the original whatsnew from the other PR describe this change as well? |
TomAugspurger
added
ExtensionArray
I think so yes, it's still in general about preserving EA dtype in concat. But I added the issue number to the exsting whatsnew. |
|
Merged master. @jreback was the discussion in #34985 (comment) resolved? |
|
yes this is fine to merge |
Closes #34095