Repository: modin-project/modin
Pull request: FIX-#5653: implement `convert_dtypes` as a full-axis operation instead of using map approach
<!--
Thank you for your contribution!
Please review the contributing docs: https://modin.readthedocs.io/en/latest/development/contributing.html
if you have questions about contributing.
-->

## What do these changes do?

<!-- Please give a short brief about these changes. -->

- [x] first commit message and PR title follow format outlined [here](https://modin.readthedocs.io/en/latest/development/contributing.html#commit-message-formatting)
  > **_NOTE:_**  If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.
- [x] passes `flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py`
- [x] passes `black --check modin/ asv_bench/benchmarks scripts/doc_checker.py`
- [x] signed commit with `git commit -s` <!-- you can amend your commit with a signature via `git commit -amend -s` -->
- [x] Resolves #5653 <!-- issue must be created for each patch -->
- [ ] tests added and passing
- [x] module layout described at `docs/development/architecture.rst` is up-to-date <!-- if you have added, renamed or removed files or directories please update the documentation accordingly -->


Problem statement:
convert_dtypes() not inferring data type of column as a whole
I have a Modin DataFrame with a sparsely populated string column, i.e. a column with a lot of `NaN` and some strings:
```
>>> df
        string_column
0                 NaN
1                 NaN
2                 NaN
3                 NaN
4                 NaN
...               ...
1133496           NaN
1133497           NaN
1133498           NaN
1133499           NaN
1133500           NaN
[1133501 rows x 1 columns]
```
The notna part:
```
>>> df.loc[df['string_column'].notna(),:]
      string_column
42          Reduced
43      Non-Reduced
44          Reduced
45      Non-Reduced
46          Reduced
...             ...
86803   Non-Reduced
86804       Reduced
86805   Non-Reduced
86846       Reduced
86847   Non-Reduced
[18126 rows x 1 columns]
```
The `dtype` of the string column is `object`:
```
>>> df.dtypes
string_column    object
dtype: object
```

I would like to have it converted into `string` using `convert_dtypes()` on the whole DataFrame (that contains also other columns not shown here, and I'm not using `astype(string`) because the data type is not known ex ante and, hence, should be inferred). However, performing `convert_dtypes()` does not change the `dtype`:
```
>>> df.convert_dtypes().dtypes
string_column    object
dtype: object
```

In pandas, this works just fine, i.e. the column is converted into `string`. I assume that in contrast to pandas, Modin does it partition-wise, and if a partition happens to contain only `NaN`, it can't determine the data type and hence the "lowest common denominator" data type for all partitions will be `object`. Performing it only on the notna part works as expected:
```
>>> df.loc[df['string_column'].notna(),:].convert_dtypes().dtypes
string_column    string
dtype: object
```

**Question:** is this expected behaviour or a bug?

Hints from issue discussion:
Hi @jsarbach , thanks for opening the issue. I think your hypothesis is correct. Since Modin operates at the partition-layer, the nulls might be throwing off the type conversion. This is a bug since we should be matching pandas in this case. 

cc: @modin-project/modin-core 
@modin-project/modin-core the easiest way to do `convert_dtypes` as a full axis operation. Any other thoughts?
That sounds right. @noloerino I remember you implemented `convert_dtypes`. Should we change it to full axis?
From rereading the discussion in #4671 I think that should be fine, though it might require some nontrivial code changes since we just implement it with algebra map operator right now.
@mvashishtha @noloerino I changed the implementation in https://github.com/modin-project/modin/pull/5885. Please review it.

Commit message: fix

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>

Patch to review:
diff --git a/modin/core/storage_formats/pandas/query_compiler.py b/modin/core/storage_formats/pandas/query_compiler.py
index 75f4ff9e0f5..25965c55274 100644
--- a/modin/core/storage_formats/pandas/query_compiler.py
+++ b/modin/core/storage_formats/pandas/query_compiler.py
@@ -1446,7 +1446,16 @@ def isin_func(df, values):
     abs = Map.register(pandas.DataFrame.abs, dtypes="copy")
     applymap = Map.register(pandas.DataFrame.applymap)
     conj = Map.register(lambda df, *args, **kwargs: pandas.DataFrame(np.conj(df)))
-    convert_dtypes = Map.register(pandas.DataFrame.convert_dtypes)
+
+    def convert_dtypes(self, *args, **kwargs):
+        new_modin_frame = self._modin_frame.apply_full_axis(
+            0,
+            lambda df: df.convert_dtypes(*args, **kwargs),
+            new_index=self._modin_frame._index_cache,
+            new_columns=self._modin_frame._columns_cache,
+        )
+        return self.__constructor__(new_modin_frame)
+
     invert = Map.register(pandas.DataFrame.__invert__, dtypes="copy")
     isna = Map.register(pandas.DataFrame.isna, dtypes=np.bool_)
     _isfinite = Map.register(
diff --git a/modin/pandas/test/dataframe/test_map_metadata.py b/modin/pandas/test/dataframe/test_map_metadata.py
index ff754fe8e9c..5a35f2b5451 100644
--- a/modin/pandas/test/dataframe/test_map_metadata.py
+++ b/modin/pandas/test/dataframe/test_map_metadata.py
@@ -638,6 +638,20 @@ def test_convert_dtypes_multiple_row_partitions():
     assert modin_result.dtypes.equals(pandas_result.dtypes)
 
 
+@pytest.mark.skipif(
+    get_current_execution() == "BaseOnPython",
+    reason="BaseOnPython cannot not have multiple partitions.",
+)
+def test_convert_dtypes_5653():
+    modin_part1 = pd.DataFrame({"col1": ["a", "b", "c", "d"]})
+    modin_part2 = pd.DataFrame({"col1": [None, None, None, None]})
+    modin_df = pd.concat([modin_part1, modin_part2])
+    assert modin_df._query_compiler._modin_frame._partitions.shape == (2, 1)
+    modin_df = modin_df.convert_dtypes()
+    assert len(modin_df.dtypes) == 1
+    assert modin_df.dtypes[0] == "string"
+
+
 @pytest.mark.parametrize("data", test_data_values, ids=test_data_keys)
 @pytest.mark.parametrize("axis", axis_values, ids=axis_keys)
 @pytest.mark.parametrize("bound_type", ["list", "series"], ids=["list", "series"])


Code review comments:
diff_hunk: @@ -1446,7 +1446,16 @@ def isin_func(df, values):
     abs = Map.register(pandas.DataFrame.abs, dtypes="copy")
     applymap = Map.register(pandas.DataFrame.applymap)
     conj = Map.register(lambda df, *args, **kwargs: pandas.DataFrame(np.conj(df)))
-    convert_dtypes = Map.register(pandas.DataFrame.convert_dtypes)
+
+    def convert_dtypes(self, *args, **kwargs):
+        new_modin_frame = self._modin_frame.apply_full_axis(
+            0,
+            lambda df: df.convert_dtypes(*args, **kwargs),
+            new_index=self._modin_frame._index_cache,
+            new_columns=self._modin_frame._columns_cache,
+        )
+        return self.__constructor__(new_modin_frame)
path: modin/core/storage_formats/pandas/query_compiler.py
text: @user1:
Can we use an algebra operator for that purpose? [`Fold` explicitly preserves](https://github.com/modin-project/modin/blob/40df200c67ef93dbbe86a3b15ab01926ff7e3faf/modin/core/dataframe/pandas/dataframe/dataframe.py#L1845-L1846) row lengths and column widths in contrasts with `.apply_full_axis` that does so implicitly and so may no always do so 
```suggestion
    convert_dtypes = Fold.register(pandas.DataFrame.convert_dtypes)
```
(fold with no axis arguments applies across 0 axis)

diff_hunk: @@ -638,6 +638,20 @@ def test_convert_dtypes_multiple_row_partitions():
     assert modin_result.dtypes.equals(pandas_result.dtypes)
 
 
+@pytest.mark.skipif(
+    get_current_execution() == "BaseOnPython",
+    reason="BaseOnPython cannot not have multiple partitions.",
+)
+def test_convert_dtypes_5653():
+    modin_part1 = pd.DataFrame({"col1": ["a", "b", "c", "d"]})
+    modin_part2 = pd.DataFrame({"col1": [None, None, None, None]})
+    modin_df = pd.concat([modin_part1, modin_part2])
+    assert modin_df._query_compiler._modin_frame._partitions.shape == (2, 1)
path: modin/pandas/test/dataframe/test_map_metadata.py
text: @user1:
neither HDK nor BaseOnPython has partitions, so let's make this check more accurate
```suggestion
    if StorageFormat.get() == "Pandas"
        assert modin_df._query_compiler._modin_frame._partitions.shape == (2, 1)
```