DEP Adding a warning in the SimpleImputer when strategy mode is constant and keep_empty_features is False #29950

ArthurCourselle · 2024-09-27T15:06:39Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Adding a warning in the SimpleImputer to prevent the user about an upcoming deprecation. This shows when the user set the keep_empty_features to False and the strategy mode is constant and the user have a column full of np.nan because it is not removed.

…ant and keep_empty_features is False

github-actions · 2024-09-27T15:08:36Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: ed415fd. Link to the linter CI: here}

glemaitre · 2024-10-01T08:03:16Z

I'll start to make a review with a couple of changes to do.

glemaitre · 2024-10-01T08:13:48Z

The current docstring of keep_empty_features should be amended to document the deprecation:

    keep_empty_features : bool, default=False
        If True, features that consist exclusively of missing values when
        `fit` is called are returned in results when `transform` is called.
        The imputed value is always `0` except when `strategy="constant"`
        in which case `fill_value` will be used instead.

        .. versionadded:: 1.2

        .. versionchanged:: 1.6
           Currently, when `keep_empty_feature=False` and `strategy="constant",
           empty features are not dropped. This behaviour will change in version
           1.8. Set `keep_empty_feature=True` to preserve this behaviour.

glemaitre · 2024-10-01T08:48:52Z

So right now, the CI is failing because we raise the warning in some tests:

FAILED impute/tests/test_impute.py::test_imputation_constant_integer - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_float[csr_matrix] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_float[csr_array] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_float[asarray] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_object[None] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_object[nan] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_object[NAN] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_object[] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_object[0] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_pandas[object] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_imputation_constant_pandas[category] - FutureWarning: When `keep_empty_features` is False and `constant` is set th...
FAILED impute/tests/test_impute.py::test_iterative_imputer_constant_fill_value - FutureWarning: When `keep_empty_features` is False and `constant` is set th...

So we should look at each case, and check if it makes sense and catch the warning / change the test or if this is a false positive and a bug in the code. I'll review the code and propose the changes.

glemaitre · 2024-10-01T09:01:24Z

So luckily, for the tests test_imputation_constant_integer, test_imputation_constant_float, test_imputation_constant_object, test_imputation_constant_pandas, we intend to test the same behaviour: check that having a column full of missing values is preserved. So we can add keep_empty_features=True in the test because this is something that we want to test right now.

However, could you add the following on the top of each test:

# TODO (1.8): check that `keep_empty_features=False` drop the
# empty features due to the behaviour change.

For the remaining test test_iterative_imputer_constant_fill_value, we can also set keep_empty_features=True.

glemaitre

OK here is a round of review.

Please add an entry to the change log at doc/whats_new/v1.6.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

This will resolve the remaining CI failure related to checking the changelog.

sklearn/impute/_base.py

sklearn/impute/tests/test_impute.py

ArthurCourselle · 2024-10-02T21:48:19Z

Thank you, I will start working on all those reviews.

…onstant, changing some tests, removing a useless test and updating the changelog

…le/scikit-learn into simple_imputer_add_warning

ArthurCourselle · 2024-10-15T18:25:20Z

I've made the requested modifications based on your reviews. Sorry that I didn't have much time to focus on it earlier. Everything should be up to date now, and please let me know if there's anything else I need to adjust. I'm waiting for the CI results.

glemaitre · 2024-10-16T14:46:10Z

Sorry that I didn't have much time to focus on it earlier.

No issue. Apparently the CI is not happy. I'll have a look at what is going on.

glemaitre · 2024-10-16T14:59:29Z

The remaining failure is in another estimator called IterativeImputer. It internally uses the SimpleImputer and is going to raise a warning. But this is a false positive because we have already some specific code that make sure that remove the empty feature (it was considered as a bug in the past).

So here, you would need to catch the warning. The diff will look like:

diff --git a/sklearn/impute/_iterative.py b/sklearn/impute/_iterative.py
index d2d47bae3f..a90a659980 100644
--- a/sklearn/impute/_iterative.py
+++ b/sklearn/impute/_iterative.py
@@ -635,6 +635,13 @@ class IterativeImputer(_BaseImputer):
 
         X_missing_mask = _get_mask(X, self.missing_values)
         mask_missing_values = X_missing_mask.copy()
+
+        # TODO (1.8): remove this once the deprecation is removed. In the meantime,
+        # we need to catch the warning to avoid false positives.
+        catch_warning = (
+            self.initial_strategy == "constant" and not self.keep_empty_features
+        )
+
         if self.initial_imputer_ is None:
             self.initial_imputer_ = SimpleImputer(
                 missing_values=self.missing_values,
@@ -642,9 +649,19 @@ class IterativeImputer(_BaseImputer):
                 fill_value=self.fill_value,
                 keep_empty_features=self.keep_empty_features,
             ).set_output(transform="default")
-            X_filled = self.initial_imputer_.fit_transform(X)
+            if catch_warning:
+                with warnings.catch_warnings():
+                    warnings.simplefilter("ignore", FutureWarning)
+                    X_filled = self.initial_imputer_.fit_transform(X)
+            else:
+                X_filled = self.initial_imputer_.fit_transform(X)
         else:
-            X_filled = self.initial_imputer_.transform(X)
+            if catch_warning:
+                with warnings.catch_warnings():
+                    warnings.simplefilter("ignore", FutureWarning)
+                    X_filled = self.initial_imputer_.transform(X)
+            else:
+                X_filled = self.initial_imputer_.transform(X)
 
         if in_fit:
             self._is_empty_feature = np.all(mask_missing_values, axis=0)
@@ -658,7 +675,7 @@ class IterativeImputer(_BaseImputer):
                 # The constant strategy has a specific behavior and preserve empty
                 # features even with ``keep_empty_features=False``. We need to drop
                 # the column for consistency.
-                # TODO: remove this `if` branch once the following issue is addressed:
+                # TODO (1.8): remove this `if` branch once the following issue is addressed:
                 # https://github.com/scikit-learn/scikit-learn/issues/29827
                 X_filled = X_filled[:, ~self._is_empty_feature]

Applying it will make the test pass.

glemaitre · 2024-10-16T14:48:19Z

sklearn/impute/_base.py

+        .. versionchanged:: 1.6
+        Currently, when `keep_empty_feature=False` and `strategy="constant"`,
+        empty features are not dropped. This behaviour will change in version
+        1.8. Set `keep_empty_feature=True` to preserve this behaviour.


This should solve the documentation failure, sphinx is quite stubborn about those :).

Suggested change

.. versionchanged:: 1.6

Currently, when `keep_empty_feature=False` and `strategy="constant"`,

empty features are not dropped. This behaviour will change in version

1.8. Set `keep_empty_feature=True` to preserve this behaviour.

.. versionchanged:: 1.6

Currently, when `keep_empty_feature=False` and `strategy="constant"`,

empty features are not dropped. This behaviour will change in version

1.8. Set `keep_empty_feature=True` to preserve this behaviour.

…mple imputer class

ArthurCourselle · 2024-10-28T08:47:11Z

Thank you ! I have corrected what you told me, however I saw when fetching the main upstream that many lines have been deleted in v.1.6.rst file. I think it is the reason why the check changelog ci is failing. Do I need to modify another file for the changelog ?

glemaitre · 2024-10-28T19:33:25Z

Indeed, we change the way we create changelog entry to have a more automated way. Now, we need to create a single file in the module folder. I push a commit to create the file in the last commit.

glemaitre

LGTM. Thanks @ArthurCourselle

We will need a second reviewer.
Maybe @adrinjalali can have a look such that we get this one in to not delay the deprecation.

ArthurCourselle · 2024-10-29T10:04:16Z

Ok thank you ! I'm looking forward for the other review then. I will look for other issues that I could work on in the future. Thanks @glemaitre for your help :)

glemaitre · 2024-10-30T10:42:17Z

Nice. Thanks @ArthurCourselle for you contribution ;)

DEP Adding a warning in the SimpleImputer when strategy mode is const…

969ad51

…ant and keep_empty_features is False

github-actions bot added the module:impute label Sep 27, 2024

Merge branch 'main' into simple_imputer_add_warning

4f806ae

glemaitre self-requested a review October 1, 2024 08:00

glemaitre reviewed Oct 1, 2024

View reviewed changes

sklearn/impute/_base.py Outdated Show resolved Hide resolved

sklearn/impute/_base.py Outdated Show resolved Hide resolved

sklearn/impute/tests/test_impute.py Outdated Show resolved Hide resolved

sklearn/impute/tests/test_impute.py Outdated Show resolved Hide resolved

ArthurCourselle added 3 commits October 15, 2024 19:54

FIX Changing the warning in the SimpleImputer when strategy mode is c…

e2083d4

…onstant, changing some tests, removing a useless test and updating the changelog

Merge branch 'simple_imputer_add_warning' of github.com:ArthurCoursel…

5911d0e

…le/scikit-learn into simple_imputer_add_warning

Merge branch 'main' into simple_imputer_add_warning

2f43912

glemaitre self-requested a review October 16, 2024 14:46

glemaitre reviewed Oct 16, 2024

View reviewed changes

ArthurCourselle added 2 commits October 28, 2024 09:35

fix: Resolving false positive test case by catching the warning in si…

b0f4a5d

…mple imputer class

Merging main upstream

b7d9e6d

glemaitre self-requested a review October 28, 2024 19:29

update changelog

fb19b5b

glemaitre approved these changes Oct 28, 2024

View reviewed changes

glemaitre added this to the 1.6 milestone Oct 28, 2024

Merge branch 'main' into simple_imputer_add_warning

ed415fd

adrinjalali approved these changes Oct 29, 2024

View reviewed changes

adrinjalali merged commit c1b1f87 into scikit-learn:main Oct 29, 2024
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEP Adding a warning in the SimpleImputer when strategy mode is constant and keep_empty_features is False #29950

DEP Adding a warning in the SimpleImputer when strategy mode is constant and keep_empty_features is False #29950

ArthurCourselle commented Sep 27, 2024

github-actions bot commented Sep 27, 2024 •

edited

Loading

glemaitre commented Oct 1, 2024

glemaitre commented Oct 1, 2024

glemaitre commented Oct 1, 2024

glemaitre commented Oct 1, 2024

glemaitre left a comment

ArthurCourselle commented Oct 2, 2024

ArthurCourselle commented Oct 15, 2024

glemaitre commented Oct 16, 2024

glemaitre commented Oct 16, 2024

glemaitre Oct 16, 2024

ArthurCourselle commented Oct 28, 2024

glemaitre commented Oct 28, 2024

glemaitre left a comment

ArthurCourselle commented Oct 29, 2024

glemaitre commented Oct 30, 2024

DEP Adding a warning in the SimpleImputer when strategy mode is constant and keep_empty_features is False #29950

DEP Adding a warning in the SimpleImputer when strategy mode is constant and keep_empty_features is False #29950

Conversation

ArthurCourselle commented Sep 27, 2024

Reference Issues/PRs

What does this implement/fix? Explain your changes.

github-actions bot commented Sep 27, 2024 • edited Loading

✔️ Linting Passed

glemaitre commented Oct 1, 2024

glemaitre commented Oct 1, 2024

glemaitre commented Oct 1, 2024

glemaitre commented Oct 1, 2024

glemaitre left a comment

Choose a reason for hiding this comment

ArthurCourselle commented Oct 2, 2024

ArthurCourselle commented Oct 15, 2024

glemaitre commented Oct 16, 2024

glemaitre commented Oct 16, 2024

glemaitre Oct 16, 2024

Choose a reason for hiding this comment

ArthurCourselle commented Oct 28, 2024

glemaitre commented Oct 28, 2024

glemaitre left a comment

Choose a reason for hiding this comment

ArthurCourselle commented Oct 29, 2024

glemaitre commented Oct 30, 2024

github-actions bot commented Sep 27, 2024 •

edited

Loading