Bug Fix: #60343 Construction of Series / Index fails from dict keys when "str" dtype is specified explicitly #60383
Conversation
|
I was able to fix the bug but I am a bit stuck on how to fix the checks. Could @jorisvandenbossche or anyone else help? Especially the unit tests. I tried to fix the pre-commit (using ruff lint fix) but every time I fixed a formatting issue, after running pre-commit it goes to the initial position before I did the fix. For the Doc build and upload check (it was giving an error for every declaration of ipython that didn't have import pandas as pd), I manually inserted it but don't know if there is an easy way. |
jorisvandenbossche
added
Strings
That should normally not have been needed. Did you get those errors locally? (in that case maybe something with the set up was wrong) |
pandas/core/dtypes/common.py
Outdated
| return ( | ||
| isinstance(arr_or_dtype, np.dtype) | ||
| and arr_or_dtype == "object" | ||
| or isinstance(arr_or_dtype, StringDtype) |
There was a problem hiding this comment.
We don't want to change the meaning of is_object_dtype to also include StringDtype. What was the reason you needed this change?
There was a problem hiding this comment.
This change was included to pass the test case "test_is_object[string-True]" because it did not recognize the string as a valid object type. It passed the 60343 test case I added but failed the existing ones. In that case, would I leave the existing test case as is with no modification even if it fails so that I an make the PR more focused?
The main issue I am having is changing something for this bug is leaded to existing test cases and I am wondering if I should only be passing the test case I added for this bug or all of the test cases?
There was a problem hiding this comment.
I don't think that test_is_object should change because of the fix that this PR is trying to do. The test already has a if using_infer_string and index.dtype == "string" check to change the expected result when using string dtype
| df = DataFrame( | ||
| { | ||
| "A": range(3), | ||
| "B": range(3), | ||
| "C": range(3) | ||
| } | ||
| ).rename(columns={"B": "A"}) | ||
| df = DataFrame({"A": range(3), "B": range(3), "C": range(3)}).rename( | ||
| columns={"B": "A"} | ||
| ) |
There was a problem hiding this comment.
It seems you included some unintended formatting changes. Maybe some setting in the IDE you are using that is conflicting with the formatting defaults in pandas?
I would recommend you to set up the pre-commit hook, which will ensure the code is formatted correctly when committing (see https://pandas.pydata.org/docs/development/contributing_codebase.html#pre-commit)
Yes when I was doing pre-commit hook locally, I saw that without ipython import it was causing failed hooks in each.rst file |
|
Hi Joris! I will make another pull request. I wanted to work on a new branch and accidentally removed my changes. |
The default behaviour (pd.Index(d.keys())) worked correctly, but explicitly setting dtype="str" raised a ValueError. The issue stemmed from dict_keys not being converted to a proper array-like structure before being passed to StringDtype, which couldn't handle such inputs.
To fix the issue:
KeyView was introduced to identify and preprocess dict_keys before passing them to Pandas internals. The keys are now converted to a list for compatibility.
Updated logic in Index and sanitize_array to map dtype="str" to StringDtype(storage="python"). Updated check_array_indexer to allow empty boolean indexers for StringArray
New test added "test_index_from_dict_keys_with_dtype" to ensure:
Default inference (pd.Index(d.keys())) works.
Explicit dtype="str" works, resulting in string[python].
Updated existing tests (test_is_object and test_empty_fancy) to handle new behaviours introduced by the fix.
After the fix both the default (pd.Index(d.keys())) and explicit (pd.Index(d.keys(), dtype="str")) cases work:
