-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
BUG: concat of bool and boolean giving object dtype #43409
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
Conversation
mzeitlin11
commented
Sep 4, 2021
- closes BUG: concat behaves differently for bool/boolean dtypes than it does for int64/Int64 #42800
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
@@ -597,6 +597,15 @@ def test_concat_preserves_extension_int64_dtype(): | |||
tm.assert_frame_equal(result, expected) | |||
|
|||
|
|||
def test_concat_bool_boolean(): |
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.
can you parameterize and also include a bool series here as a test (e.g. no boolean, just plain bool)
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.
Have added other combinations
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.
lgtm. comment for possible followup
# Handle only boolean + np.bool_ -> boolean, since other cases like | ||
# Int64 + boolean -> Int64 will be handled by the other type | ||
if all( | ||
isinstance(t, BooleanDtype) |
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.
is_bool_dtype should work here no?
that said i think we are using mostly this type of pattern, which i am actually not much of a fan of. so ok for this PR. but we should ideally be using the common accessors rather than writing new ones for EA types, can you open an issue about this.
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.
Ok, will do