BUG: convert_dtypes incorrectly converts byte strings to strings in 1.3+
State: closed

- [x] closes #43183
- [x] tests added / passed in the test_convert_dtypes.py file
- [x] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#code-standards) for how to run them
- [x] whatsnew entry

Added extra try except block to identify the correct type to solve the problem.

Labels:
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: Regression
description: Functionality that used to work in a prior pandas version
name: Strings
description: String extension data type and string data

Comments:
Hello @shubham11941140! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:





There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2021-10-17 02:14:48 UTC
needs scrutiny in the sense
Actually the problem is I am unable to build my pytest as I am getting E   
``` ModuleNotFoundError: No module named 'pandas._libs.interval' error ```
Please help me to solve this, then I can run local tests.
Please can you help me to compete my build so I can move to the testing.
The code segment passes the testcase in pytest successfully. Please review.
I am unable to understand that in which other files are the casts taking place and to fix them.
cast.py is the only place where the actual cast takes place and the inferred_dtype takes place, in series.py and generic.py only functions are called with the necessary arguments where no other change is required.
This is handled a few lines below in the if conditions, you have to adjust there
Few lines below the if condition in which particular file?

Same file.

You have to check where bytes is cast to string
Do you mean to say that in cast.py, there is a place where bytes is cast to string?
Yes. Please step through with the debugger to see what takes place
Please inform if any more changes are required.
> pls merge master as well

Please elaborate on this. What exactly do I need to merge?
you have a conflict, always merge upstream/master when pushing
Thanks @shubham11941140 for the PR. I'm not convinced that the pandas StringArray should allow btyes objects. Do we test this anywhere? I'm not therefore convinced that the issue is a regression. pandas StringArray is experimental so not bound by the version policy on api changes.

I think maybe there should be more discussion in the issue since there has been no response to https://github.com/pandas-dev/pandas/issues/43183#issuecomment-905786200

1.3.3 is scheduled for tomorrow, so changing the milestone on this PR to 1.3.4 to allow more time for discussion.
In cast.py, I have compared the bytes objects and prevents it's conversion into string, which is verified by the pytest.

You mean to say that ```df.astype(str)``` should not be allowed and I should raise an exception.

I feel that the issue of the bug is that convert_dtypes should not allow this, which I have prevented.

The ```df.astype(str)``` should be another issue, so I feel that instead of mixing it with this, I can create a separate PR and solve it, where we can have a discussion on this, because ```astype()``` cast does not use the convert_dtypes function.
> The df.astype(str) should be another issue, so I feel that instead of mixing it with this, I can create a separate PR and solve it, where we can have a discussion on this, because astype() cast does not use the convert_dtypes function.

this IS allowed. what is not is `df.astype(bytes)`
> this IS allowed

and should convert bytes objects in an object array to python strings?

The expected output described in the issue is a pandas StringArray containing bytes objects and the issue is titled `convert_dtypes incorrectly converts byte strings to strings in 1.3+`

Should a StringArray be allowed to hold bytes objects?

code sample
```
print(pd.__version__)
byte_str = b"binary-string"
df = pd.DataFrame(
    data={
        "data": byte_str,
    },
    index=[0],
)

result = df.astype("string")
print(result)
print(result.dtypes)
item = result.iloc[0, 0]
print(item)
print(type(item))

result2 = df.convert_dtypes()
print(result2)
print(result2.dtypes)
item2 = result2.iloc[0, 0]
print(item2)
print(type(item2))
```
```
1.2.5
               data
0  b'binary-string'
data    string
dtype: object
b'binary-string'
<class 'str'>
               data
0  b'binary-string'
data    object
dtype: object
b'binary-string'
<class 'bytes'>
```
```
1.3.3
               data
0  b'binary-string'
data    string
dtype: object
b'binary-string'
<class 'str'>
               data
0  b'binary-string'
data    string
dtype: object
b'binary-string'
<class 'str'>
```

I believe that the new/current behavior is correct. The old behavior where a pandas StringArray contained a bytes object is IMO a bug.

---

BUT I also think that the bytes object if represents ascii should be converted to a plain string without the b prefix and quotes, but I think that's a different issue.

hence I expect the output to be.

```
               data
0  binary-string
data    string
dtype: object
binary-string
<class 'str'>
               data
0  binary-string
data    string
dtype: object
binary-string
<class 'str'>
```
> The expected output described in the issue is a pandas StringArray containing bytes objects and the issue is titled `convert_dtypes incorrectly converts byte strings to strings in 1.3+`

my mistake. I read between the lines. The author did not explicitly mention that the expected output was a StringArray. I assumed this because the author was calling convert_dtypes. Looking at the issue again, I believe the expected output is convert_dtypes should be a no-op. 

So yes we could perhaps treat this as a regression but IMO convert_dtypes should aim to return nullable dtypes where possible.
> Should a StringArray be allowed to hold bytes objects?

no, i wouldn't allow this in general

> 
> code sample
> 
> ```
> print(pd.__version__)
> byte_str = b"binary-string"
> df = pd.DataFrame(
>     data={
>         "data": byte_str,
>     },
>     index=[0],
> )
> 
> result = df.astype("string")
> print(result)
> print(result.dtypes)
> item = result.iloc[0, 0]
> print(item)
> print(type(item))
> 
> result2 = df.convert_dtypes()
> print(result2)
> print(result2.dtypes)
> item2 = result2.iloc[0, 0]
> print(item2)
> print(type(item2))
> ```
> 
> ```
> 1.2.5
>                data
> 0  b'binary-string'
> data    string
> dtype: object
> b'binary-string'
> <class 'str'>
>                data
> 0  b'binary-string'
> data    object
> dtype: object
> b'binary-string'
> <class 'bytes'>
> ```
> 
> ```
> 1.3.3
>                data
> 0  b'binary-string'
> data    string
> dtype: object
> b'binary-string'
> <class 'str'>
>                data
> 0  b'binary-string'
> data    string
> dtype: object
> b'binary-string'
> <class 'str'>
> ```
> 
> I believe that the new/current behavior is correct. The old behavior where a pandas StringArray contained a bytes object is IMO a bug.
> 
> BUT I also think that the bytes object if represents ascii should be converted to a plain string without the b prefix and quotes, but I think that's a different issue.

> 
> hence I expect the output to be.
> 
> ```
>                data
> 0  binary-string
> data    string
> dtype: object
> binary-string
> <class 'str'>
>                data
> 0  binary-string
> data    string
> dtype: object
> binary-string
> <class 'str'>
> ```

In the above 3 code samples when I run the same test case of this file in the convert_dtypes, I get this output. I completely agree with @simonjayhawkins. This should hold. As a matter of fact, I have aldready done these 3 previously and committed them on this PR, so these test cases in pytest for the cast.py file is fine.

> Should a StringArray be allowed to hold bytes objects?

The answer should be "NO", otherwise this conversion would have been previously correct and there would be no meaning as classifying this as an issue and opening my PR. If this would be the case, then the initial behaviour would be correct and there is no point in solving this BUG.

> Should convert bytes objects in an object array to python strings?

Ideally "NO", the same above stated reply will apply.

> > The expected output described in the issue is a pandas StringArray containing bytes objects and the issue is titled `convert_dtypes incorrectly converts byte strings to strings in 1.3+`
> 
> my mistake. I read between the lines. The author did not explicitly mention that the expected output was a StringArray. I assumed this because the author was calling convert_dtypes. Looking at the issue again, I believe the expected output is convert_dtypes should be a no-op.
> 


I did not mention that the output should be a StringArray. It should be a bytes object only because the conversion is incorrect.

> So yes we could perhaps treat this as a regression but IMO convert_dtypes should aim to return nullable dtypes where possible.
>

The code has returned the dtype to be a pandas object, which is right to classify it. Keeping it as a nullable dtype will not be of much use as later it could crop up another problem like this and we will be solving it later. Giving it a specified dtype would be a wise plan, it will prevent implicit conversions.
```df.astype(bytes)``` does not make sense as it is invalid but assigns dtype to bytes so I am able to compare bytes, if you check the ```df.info``` during the test case run.

```df``` is treated as an object before the function is called
In testing, I can compare them as:
1.  Strings (by decoding the binary objects into plaintext/ascii format) - No valid dtype issue.
2.  Byte Objects (```df.astype(bytes)```  - dtype issue as previously mentioned by @jreback) (invalid)
3.  Objects (no fixed dtype) - Treating all the entities as objects during the comparison - No valid dtype issue.

Please mention that which among the three, I must do, to complete the test function.
for the code sample in https://github.com/pandas-dev/pandas/pull/43199#issuecomment-918703946 the current state of the PR gives

```
1.4.0.dev0+708.g766a30fa85
               data
0  b'binary-string'
data    string
dtype: object
b'binary-string'
<class 'str'>
               data
0  b'binary-string'
data    |S13
dtype: object
b'binary-string'
<class 'numpy.bytes_'>
```

So we are therefore not yet reverting to 1.2.5 behavior.

As @jreback mentions in https://github.com/pandas-dev/pandas/pull/43199#discussion_r706130833 the expected is `df`. So although we would normally create an "expected" explicitly for a test, in this case if we are reverting to 1.2.5 behavior, the `convert_dtypes` call is a no-op. Hence we don't need to construct an expected and just do `assert_frame_equal(result, df)` which currently gives...

```
AssertionError: Attributes of DataFrame.iloc[:, 0] (column name="data") are different

Attribute "dtype" are different
[left]:  |S13
[right]: object
```

---

In https://github.com/pandas-dev/pandas/pull/43199#issuecomment-918514924 I didn't include a constructor example...

```
byte_str = b"binary-string"
df = pd.DataFrame(
    data={"data": byte_str},
    index=[0],
    dtype="string",
)
```

which gives the same as `astype` and also the same as `convert_dtypes` on master.

from the docs https://pandas.pydata.org/pandas-docs/dev/reference/api/pandas.DataFrame.convert_dtypes.html

```
Parameters
infer_objects : bool, default True
Whether object dtypes should be converted to the best possible types.

convert_string : bool, default True
Whether object dtypes should be converted to StringDtype().
```

The whole point of `convert_dtypes` is to "Convert columns to best possible dtypes using dtypes supporting pd.NA."

For consistency, I think convert_types should do the same as the specifying `dtype` in the constructor and the same as `astype`. This is currently the behavior on master so I am +1 on leaving it as it is (i.e. not fixing the regression.)

---

There is also the issue of the stringification of the bytes object instead of encoding ASCII to python unicode strings. Again I think the constructor, `astype` and `convert_dtypes` should be consistent so changing this would not be part of a backport, but could be a change for 1.4

@simonjayhawkins, do you mean to say that is fine to merge this PR, without any further changes?

Leaving the issue as it is will not solve the problem as it will create issues in the future versions?

If I create the ```df``` in a manner as previously stated, I am able to assign the proper dtype, however the test function is not being verified the way @jreback is expecting?

Please tell me what do I do next?
> @simonjayhawkins, do you mean to say that is fine to merge this PR, without any further changes?

no. The test needs to be `assert_frame_equal(result, df)` without the expected. The behavior should be the same as 1.2.5

> Leaving the issue as it is will not solve the problem as it will create issues in the future versions?

The behavior on master is consistent with astype("string") and the Constructor. In the future I would expect to not stringify the bytes objects but convert ASCII printable strings to python Strings.

StringArray is experimental and to help evolve the api for native string array. We have a pyarrow string array and pyarrow string functions have support for ASCII strings (maybe performance benefits to this)

If this is not the future, and we revert to the 1.2.5 behavior for convert_dtypes, then I would also expect astype("string") to raise instead of stringify the bytes representation.

from https://pandas.pydata.org/pandas-docs/dev/reference/api/pandas.DataFrame.convert_dtypes.html

> For object-dtyped columns, if infer_objects is True, use the inference rules as during normal Series/DataFrame construction. Then, if possible, convert to StringDtype, BooleanDtype or an appropriate integer or floating extension type, otherwise leave as object.

my understanding is that "if possible" the object array should be converted. If we revert to 1.2.5, we are considering the conversion not possible? By implication, if the conversion is "not possible", then astype("string") should raise?

> Please tell me what do I do next?

either revert to 1.2.5 behavior or maybe allow time for others to comment on the issue.

``` def test_convert_byte_string_dtype(self):
        # GH-43183
        byte_str = b"binary-string"
    
        df = pd.DataFrame(data={"A": byte_str}, index=[0])
        result = df.convert_dtypes()
        expected = df
>       tm.assert_frame_equal(result, expected)
E       AssertionError: Attributes of DataFrame.iloc[:, 0] (column name="A") are different
E       
E       Attribute "dtype" are different
E       [left]:  |S13
E       [right]: object
```

i see the problem. the expected is correct, but this doesn't fix the bug. we *should* not have a dtype like this.
How am I supposed to solve this? Please give me some insight, I was able to compare bytes as you previously requested,, but the incorrect conversion is removed from the code. The testcase is the issue of failure.
> i see the problem. the expected is correct, but this doesn't fix the bug. we _should_ not have a dtype like this.

In this case, what do I do, bytes is not supported as a dtype, hence I am using this method to solve the bug. It is removed the incorrect type cast.


> How am I supposed to solve this? Please give me some insight

see https://github.com/pandas-dev/pandas/issues/43183#issuecomment-903913947

The regression was caused by #41622, do we know exactly what change in that PR caused the regression? Is https://github.com/pandas-dev/pandas/issues/43183#issuecomment-904114296 correct? 

At first glance there appears there was a significant refactor to convert_dtypes in #41622, predominantly to return dtype objects but is there also another change that causes the reported regression?