REGR: ensure numpy fixed-size binary/string arrays are converted to object dtype for Index creation #57995
Conversation
….2.1
While using the function set_index with parameter inplace=True, the function would try and create a new index where its dtype would be a FLS S{value} dtype, which was not recognized by the function _dtype_to_subclass and raised a NotImplementedError. That said , by adding a verification that recognizes FLS dtype , the index is created successfully and the function executes properly.
PF2100
force-pushed
the
57645-cannot-detect-FLSdtype-in-base-_dtype_to_subclass
branch
from
39fcc6a
to
4b27a30
Compare
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
jorisvandenbossche
changed the title
jorisvandenbossche
added
Dtype Conversions
There was a problem hiding this comment.
Thanks a lot for working on this, and apologies for the lack of reviews!
The original PR that introduced this change edited the array_tuplesafe function (#57139), which is called inside Index.__new__ before calling _dtype_to_subclass. So if we would change array_tuplesafe to still return object dtype instead of numpy S/U, that would fix it before getting to _dtype_to_subclass.
The current fix also essentially allows to create an Index with an underlying numpy array with S6 dtype, while I think we want to restore the behaviour of converting to object dtype (I think my suggestion above should do that)
Other comment: could you also add a test for direct Index constructor from a Series object (i.e. pd.Index(pd.Series(arr))), because that is affected as well, in addition to set_index
| string_length = 6 | ||
| in_dtype, df_name = f"S{string_length}", "fruit" | ||
| data = ["apple", "banana", "orange", "grape"] | ||
|
|
||
| # Create array with FLS(|S{value}) dtype | ||
| arr = np.array(data, dtype=in_dtype) |
There was a problem hiding this comment.
For the purpose of the tests, we can simplify the snippet from the issue report a bit
| string_length = 6 | |
| in_dtype, df_name = f"S{string_length}", "fruit" | |
| data = ["apple", "banana", "orange", "grape"] | |
| # Create array with FLS(|S{value}) dtype | |
| arr = np.array(data, dtype=in_dtype) | |
| # Create array with FLS(|S{value}) dtype | |
| arr = np.array( ["apple", "banana", "orange", "grape"], dtype="S6") |
There was a problem hiding this comment.
Thank you very much for the sugestion and I will change the test. Regarding the asarray_tuplesafe, I went into a little more detail in this PR's issue (#57645) but looking back into the original PR, the line elif isinstance(values, ABCSeries) was added which allows for any Series to keep their original types and values without performing any check if their type is compatible or not.
Could a possible solution be something of the sort:
elif isinstance(values, ABCSeries) and values.dtype.kind != "S":
return values_valuesBy refactoring this line, the elif will fail if the values have dtype ="S" and creates a new array with dtype object as it used to. Before I proceed, could I know your thoughts on the suggestion I've made?
doc/source/whatsnew/v3.0.0.rstfile if fixing a bug or adding a new feature.While using the function set_index with parameter inplace=True, the function would try and create a new index where its dtype would be a FLS
S{value}dtype, which was not recognized by the function_dtype_to_subclassand raised a NotImplementedError. That said , by adding a verification that recognizes FLS dtype , the index is created successfully and the function executes properly.