BUG: is_string_dtype(pd.Index([], dtype='O')) returns False #54997
Conversation
natmokval
added
Bug
Dtype Conversions
|
@jbrockmendel, could you please take a look at this PR? |
pandas/tests/dtypes/test_dtypes.py
Outdated
|
|
||
|
|
||
| def test_empty_object_array_is_string_dtype(): | ||
| # GH #54661 |
There was a problem hiding this comment.
nitpick can you remove the space after GH. i like the pattern GH#12345 bc it is something i grep for
There was a problem hiding this comment.
sure, done. I agree, it's better to have all GH refers look alike
pandas/tests/dtypes/test_dtypes.py
Outdated
| @@ -1212,3 +1212,8 @@ def test_multi_column_dtype_assignment(): | |||
|
|
|||
| df["b"] = 0 | |||
| tm.assert_frame_equal(df, expected) | |||
|
|
|||
|
|
|||
| def test_empty_object_array_is_string_dtype(): | |||
There was a problem hiding this comment.
hmm looks like we have tests for is_string_dtype in a couple of places. one of these days might be worth checking if there is a reason for that and if not, refactoring
There was a problem hiding this comment.
Thank you for the comment. I think, I found a better place for my test: pandas/tests/dtypes/test_common.py. I combined my test with another one and added a parameterization.
doc/source/whatsnew/v2.2.0.rst
Outdated
| @@ -169,6 +169,7 @@ Bug fixes | |||
| ~~~~~~~~~ | |||
| - Bug in :class:`AbstractHolidayCalendar` where timezone data was not propagated when computing holiday observances (:issue:`54580`) | |||
| - Bug in :class:`pandas.core.window.Rolling` where duplicate datetimelike indexes are treated as consecutive rather than equal with ``closed='left'`` and ``closed='neither'`` (:issue:`20712`) | |||
| - Bug in :func:`is_all_strings` while checking object array with no elements is of the string dtype (:issue:`54661`) | |||
There was a problem hiding this comment.
is the bug in is_all_strings or is_string_dtype?
There was a problem hiding this comment.
I think, the bug is in is_all_strings, but we don’ t have this function in api and users see the bug while calling is_string_dtype. That is why I replaced Bug in :func:is_all_strings with: Bug in :func:is_string_dtype.
Looks like we don’t have a test for is_all_strings, do we need one?
There was a problem hiding this comment.
i meant the user-facing bug. in the docs we generally only reference public functions/methods
There was a problem hiding this comment.
okay, thank you. I removed from doc/source/whatsnew/v2.2.0.rst my line.
There was a problem hiding this comment.
@jbrockmendel, I updated the PR. Could you please take a look at it.
There was a problem hiding this comment.
i think we still want a note, it just needs to refer to is_string_dtype, which I think is public
There was a problem hiding this comment.
Yeah, is_string_dtype is a public function, I added the note to v2.1.1.rst. I think my change is a minor and we can put it in the version 2.1.1
doc/source/whatsnew/v2.1.1.rst
Outdated
| @@ -34,6 +34,7 @@ Fixed regressions | |||
| Bug fixes | |||
| ~~~~~~~~~ | |||
| - Fixed bug for :class:`ArrowDtype` raising ``NotImplementedError`` for fixed-size list (:issue:`55000`) | |||
| - Fixed bug in :func:`is_string_dtype` while checking object array with no elements is of the string dtype (:issue:`54661`) | |||
There was a problem hiding this comment.
I think you'll need this to be
:func:`pandas.api.types.is_string_dtype`
and to move this to 2.2
There was a problem hiding this comment.
thanks, I corrected the note in whatsnew and moved it to 2.2.0
|
@jbrockmendel, I corrected my PR, could you please take a look at the update? |
|
LGTM. pls merge main and ping on green |
thanks, I merged main, ci - green |
|
thanks @natmokval |
False#54661doc/source/whatsnew/v2.2.0.rstCorrected the definition of
is_all_strings. Now for object arrays, which have no itemsis_string_dtypereturnsTrueinstead ofFalse.