Repository: scikit-learn/scikit-learn
Pull request: FIX handle inconsistence between `fill_value` and `X` dtype in `SimpleImputer`
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #28309


#### What does this implement/fix? Explain your changes.
Promote input type during `SimpleImputer` transform to be compatible with the `fill_value` argument when using mode `constant`.

We use `numpy.min_scalar_type` on `fill_value` and `numpy.promote_types` for finding the simplest type compatible with input and `fill_value`. The goal is to promote integer input to float if `fill_value` is float, but not to promote the input to a higher precision float if it's already a float.

@glemaitre 


Problem statement:
SimpleImputer silently cast fill values to integer when the input is of integer type
### Describe the bug

Fitting the SimpleImputer on an integer array silently cast the float `fill_value` values to integer. If `fill_value` is nan, nothing is imputed but a warning is raise:
`RuntimeWarning: invalid value encountered in cast
  multiarray.copyto(a, fill_value, casting='unsafe')`

### Steps/Code to Reproduce

```
from sklearn.impute import SimpleImputer
import numpy as np
a = np.array([0, 0, 0, 1]).reshape(-1, 1)
si = SimpleImputer(missing_values=0,
                    fill_value=3.2,
                    strategy="constant").set_output()

si.fit_transform(a)
```

```
from sklearn.impute import SimpleImputer
import numpy as np
a = np.array([0, 0, 0, 1]).reshape(-1, 1)
si = SimpleImputer(missing_values=0,
                    fill_value=np.nan,
                    strategy="constant").set_output()

si.fit_transform(a)
```

### Expected Results

Either raising an error (or a warning for the first case), or casting the array to float.
```
array([[3.2],
       [3.2],
       [3.2],
       [1.]])
```

```
array([[nan],
       [nan],
       [nan],
       [1.]])
```

### Actual Results

No warning and output:
```
array([[3],
       [3],
       [3],
       [1]])
```
Warning: `RuntimeWarning: invalid value encountered in cast
  multiarray.copyto(a, fill_value, casting='unsafe')` and output:
```
array([[0],
       [0],
       [0],
       [1]])
```

### Versions

```shell
System:
    python: 3.9.18 | packaged by conda-forge | (main, Aug 30 2023, 03:53:08)  [Clang 15.0.7 ]
executable: /Users/leo/mambaforge/envs/tabular-benchmark/bin/python
   machine: macOS-12.6.5-arm64-arm-64bit

Python dependencies:
      sklearn: 1.2.2
          pip: 23.3.1
   setuptools: 68.2.2
        numpy: 1.26.2
        scipy: 1.10.1
       Cython: None
       pandas: 2.1.4
   matplotlib: 3.8.0
       joblib: 1.2.0
threadpoolctl: 3.2.0

Built with OpenMP: True

threadpoolctl info:
       user_api: blas
   internal_api: openblas
    num_threads: 8
         prefix: libopenblas
       filepath: /Users/leo/mambaforge/envs/tabular-benchmark/lib/python3.9/site-packages/numpy/.dylibs/libopenblas64_.0.dylib
        version: 0.3.23.dev
threading_layer: pthreads
   architecture: armv8

       user_api: blas
   internal_api: openblas
    num_threads: 8
         prefix: libopenblas
       filepath: /Users/leo/mambaforge/envs/tabular-benchmark/lib/libopenblas.0.dylib
        version: 0.3.24
threading_layer: openmp
   architecture: VORTEX

       user_api: openmp
   internal_api: openmp
    num_threads: 8
         prefix: libomp
       filepath: /Users/leo/mambaforge/envs/tabular-benchmark/lib/libomp.dylib
        version: None

       user_api: openmp
   internal_api: openmp
    num_threads: 8
         prefix: libomp
       filepath: /Users/leo/mambaforge/envs/tabular-benchmark/lib/python3.9/site-packages/xgboost/.dylibs/libomp.dylib
        version: None
```

Hints from issue discussion:
Indeed this is bad. We probably introduced the bug here: https://github.com/scikit-learn/scikit-learn/pull/22063

I assume that we need to make a casting rule between the dtype of `X` seen at `fit` and the `fill_value` parameter.

@LeoGrin do you fill like making a fix. Basically we need a the casting rule to apply on `_fit_dtype`. I assume that the right way to go is be always go for more precision.
Probably we can use https://numpy.org/doc/stable/reference/generated/numpy.promote_types.html as casting rule.
> @LeoGrin do you fill like making a fix. Basically we need a the casting rule to apply on `_fit_dtype`. I assume that the right way to go is be always go for more precision.

Thanks for the reply! I can try to do a fix next week :)

Commit message: update changelog

Patch to review:
diff --git a/doc/whats_new/v1.4.rst b/doc/whats_new/v1.4.rst
index 2baa45e88a115..5fa6b934f6f3e 100644
--- a/doc/whats_new/v1.4.rst
+++ b/doc/whats_new/v1.4.rst
@@ -152,6 +152,14 @@ Changelog
   works with pandas copy-on-write mode enabled.
   :pr:`28233` by :user:`John Hopfensperger <s-banach>`.
 
+
+:mod:`sklearn.impute`
+.....................
+
+- |Fix|: :class:`impute.SimpleImputer` now raises an error instead of casting `fill_value` to an integer
+  when input_data is an integer but `fill_value` is a float.
+  :pr:`28365` by :user:`Leo Grinsztajn <LeoGrin>`.
+
 .. _changes_1_4:
 
 Version 1.4.0
diff --git a/sklearn/impute/_base.py b/sklearn/impute/_base.py
index dff39d4734554..439bb218bda61 100644
--- a/sklearn/impute/_base.py
+++ b/sklearn/impute/_base.py
@@ -363,6 +363,15 @@ def _validate_input(self, X, in_fit):
                 "with an object dtype.".format(X.dtype)
             )
 
+        # missing_values = 0 not allowed with sparse data as it would
+        # force densification
+        if sp.issparse(X) and self.missing_values == 0:
+            raise ValueError(
+                "Imputation not possible when missing_values "
+                "== 0 and input is sparse. Provide a dense "
+                "array instead."
+            )
+
         return X
 
     @_fit_context(prefer_skip_nested_validation=True)
@@ -395,32 +404,17 @@ def fit(self, X, y=None):
         else:
             fill_value = self.fill_value
 
-        # fill_value should be numerical in case of numerical input
-        if (
-            self.strategy == "constant"
-            and X.dtype.kind in ("i", "u", "f")
-            and not isinstance(fill_value, numbers.Real)
-        ):
-            raise ValueError(
-                "'fill_value'={0} is invalid. Expected a "
-                "numerical value when imputing numerical "
-                "data".format(fill_value)
-            )
-
-        if sp.issparse(X):
-            # missing_values = 0 not allowed with sparse data as it would
-            # force densification
-            if self.missing_values == 0:
+        if self.strategy == "constant":
+            if not np.can_cast(type(fill_value), X.dtype, "same_kind"):
                 raise ValueError(
-                    "Imputation not possible when missing_values "
-                    "== 0 and input is sparse. Provide a dense "
-                    "array instead."
-                )
-            else:
-                self.statistics_ = self._sparse_fit(
-                    X, self.strategy, self.missing_values, fill_value
+                    f"fill_value={fill_value} cannot be cast to the input dtype"
+                    f" {X.dtype}"
                 )
 
+        if sp.issparse(X):
+            self.statistics_ = self._sparse_fit(
+                X, self.strategy, self.missing_values, fill_value
+            )
         else:
             self.statistics_ = self._dense_fit(
                 X, self.strategy, self.missing_values, fill_value
@@ -557,6 +551,13 @@ def transform(self, X):
         X = self._validate_input(X, in_fit=False)
         statistics = self.statistics_
 
+        if self.strategy == "constant":
+            if not np.can_cast(statistics.dtype, X.dtype, "same_kind"):
+                raise ValueError(
+                    f"fill_value={statistics.item()} cannot be cast to the input dtype"
+                    f" {X.dtype}"
+                )
+
         if X.shape[1] != statistics.shape[0]:
             raise ValueError(
                 "X has %d features per sample, expected %d"
diff --git a/sklearn/impute/tests/test_impute.py b/sklearn/impute/tests/test_impute.py
index 2128c796e4800..6a348bb6bd43e 100644
--- a/sklearn/impute/tests/test_impute.py
+++ b/sklearn/impute/tests/test_impute.py
@@ -400,7 +400,7 @@ def test_imputation_constant_error_invalid_type(X_data, missing_value):
     X = np.full((3, 5), X_data, dtype=float)
     X[0, 0] = missing_value
 
-    with pytest.raises(ValueError, match="imputing numerical"):
+    with pytest.raises(ValueError, match="cannot be cast to the input dtype"):
         imputer = SimpleImputer(
             missing_values=missing_value, strategy="constant", fill_value="x"
         )
@@ -1744,3 +1744,57 @@ def test_imputation_custom(csc_container):
     imputer = SimpleImputer(missing_values=np.nan, strategy=np.min)
     X_trans = imputer.fit_transform(csc_container(X))
     assert_array_equal(X_trans.toarray(), X_true)
+
+
+def test_type_inconsistence_fill_value():
+    """Check that the type is consistent between the input and the fill_value"""
+
+    # int input and float fill_value should raise an error
+    fill_value = 1.5
+    for input_type in [np.int64, np.int32]:
+        X = np.array([1, 2, 3], dtype=input_type).reshape(-1, 1)
+        imputer = SimpleImputer(
+            strategy="constant", fill_value=fill_value, missing_values=2
+        )
+        with pytest.raises(
+            ValueError,
+            match=(
+                rf"fill_value={fill_value} cannot be cast to the input dtype {X.dtype}"
+            ),
+        ):
+            imputer.fit(X)
+        X_float = np.array([1, 2, 3], dtype=np.float64).reshape(-1, 1)
+        imputer.fit(X_float)
+        with pytest.raises(
+            ValueError,
+            match=(
+                rf"fill_value={fill_value} cannot be cast to the input dtype {X.dtype}"
+            ),
+        ):
+            imputer.transform(X)
+
+    # float input and float fill_value and
+    # float input and int fill_value should not raise an error
+    for input_type in [np.float64, np.float32]:
+        for fill_value in [1.5, 1]:
+            X = np.array([1, 2, 3], dtype=input_type).reshape(-1, 1)
+            imputer = SimpleImputer(
+                strategy="constant", fill_value=fill_value, missing_values=2
+            )
+            imputer.fit_transform(X)
+
+    # object input and any fill_value should not raise an error
+    X = np.array(["a", "b", "c"], dtype="O").reshape(-1, 1)
+    for fill_value in [1, 1.5, "a"]:
+        imputer = SimpleImputer(
+            strategy="constant", fill_value=fill_value, missing_values="b"
+        )
+        imputer.fit_transform(X)
+
+    # float32 input and float64 fill_value should not raise an error
+    X = np.array([1, 2, 3], dtype=np.float32).reshape(-1, 1)
+    fill_value = np.float64(1.5)
+    imputer = SimpleImputer(
+        strategy="constant", fill_value=fill_value, missing_values=2
+    )
+    imputer.fit_transform(X)


Code review comments:
diff_hunk: @@ -152,6 +152,14 @@ Changelog
   works with pandas copy-on-write mode enabled.
   :pr:`28233` by :user:`John Hopfensperger <s-banach>`.
 
+
+:mod:`sklearn.impute`
path: doc/whats_new/v1.4.rst
text: @user1:
You can move this section under inspection. We will reorder the rest properly for the release.