[MRG]Fix simple imputer for string input #17526

yagi-3 · 2020-06-07T23:03:33Z

Reference Issues/PRs

Fixes #17525

What does this implement/fix?

Fix to accept list of strings in SimpleImputer with strategy="most_frequent" or "constant"
Add unittest

Any other comments?

If this PR is not proper, please tell me. I will close:)

alfaro96 · 2020-06-08T12:59:55Z

sklearn/impute/tests/test_impute.py

+
+
+@pytest.mark.parametrize(
+    "strategy, X, expected_X",
+    [("most_frequent", [['a'], [np.nan]],
+      np.array([['a'], ['a']], dtype=np.object)),
+     ("constant", [['a'], [np.nan]],
+      np.array([['a'], ['missing_value']], dtype=np.object))]
+)
+def test_simple_imputation_for_string(strategy, X, expected_X):
+    imputer = SimpleImputer(strategy=strategy)
+    X_trans = imputer.fit_transform(X)
+
+    assert_array_equal(X_trans, expected_X)


For consistency with current naming, I would divide this function to test_imputation_most_frequent_string_list and test_imputation_constant_string_list, including near to similar testing functions (e.g., test_imputation_most_frequent_objects and test_imputation_constant_object).

Moreover, do not test X and expected_X as ndarray with dtype=object, since they are already tested in test_imputation_most_frequent_objects and test_imputation_constant_object.

alfaro96 · 2020-06-08T13:03:45Z

sklearn/impute/_base.py

@@ -228,7 +228,11 @@ def _validate_input(self, X, in_fit):
                                                        self.strategy))

        if self.strategy in ("most_frequent", "constant"):
-            dtype = None
+            if isinstance(X, (list, tuple)) and \


Since tuple is not a typical use case (nor array-like), I would remove this data type.

alfaro96 · 2020-06-08T13:06:53Z

sklearn/impute/_base.py

-            dtype = None
+            if isinstance(X, (list, tuple)) and \
+               any(isinstance(elem, str) for row in X for elem in row):
+                dtype = np.object


For this particular case, I prefer object instead of np.object (similarly to testing functions). Moreover, adding a comment why is this data type required would be helpful.

alfaro96

LGTM. Thanks @yagi-3!

yagi-3 · 2020-06-08T14:46:27Z

@alfaro96 Thank you for your quick and kind suggestions!
I learned a lot and appreciate the opportunity!:smile:

thomasjpfan

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

sklearn/impute/tests/test_impute.py

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

yagi-3 · 2020-06-09T05:14:48Z

@thomasjpfan Thank you for the suggestions!

I added a change log. This is my first time. I tried to follow other entries but still worried that I may make mistakes. Any suggestion is welcomed:)

yagi-3 · 2020-06-09T05:18:43Z

@alfaro96 I'd like to add your name to change log (5ff61f2). Is it ok?

alfaro96 · 2020-06-09T07:13:05Z

@alfaro96 I'd like to add your name to change log (5ff61f2). Is it ok?

It would be a pleasure. Thanks @yagi-3 for the credit!

rth

Thanks @yagi-3 ! Just one comment that tests could be parametrized, otherwise LGTM.

sklearn/impute/tests/test_impute.py

Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>

rth

Thanks!

reshamas · 2020-06-16T19:41:59Z

applied to #DataUmbrella sprint

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>

yagi-3 added 2 commits June 8, 2020 07:20

Fix SimpleImputer for string input

903e374

Add unittest SimpleImputer for string input

d6c541d

github-actions bot added the module:impute label Jun 7, 2020

yagi-3 changed the title ~~Fix simple imputer for str input~~ Fix simple imputer for string input Jun 7, 2020

yagi-3 changed the title ~~Fix simple imputer for string input~~ [WIP]Fix simple imputer for string input Jun 7, 2020

yagi-3 changed the title ~~[WIP]Fix simple imputer for string input~~ [MRG]Fix simple imputer for string input Jun 8, 2020

yagi-3 mentioned this pull request Jun 8, 2020

ValueError in SimpleImputer for string input with strategy='most_frequent'/'constant' #17525

Closed

alfaro96 reviewed Jun 8, 2020

View reviewed changes

yagi-3 added 5 commits June 8, 2020 22:34

Remove tuple from acceptable input

b26cea9

Change dtype from np.object to object

f4e1bff

Add comment why dtype=object when input is a list of strings

e3b0772

Divide test_simple_imputation_for_string to two tests

962df65

Rearrange locations of new tests to related tests

3a3109b

alfaro96 approved these changes Jun 8, 2020

View reviewed changes

adrinjalali approved these changes Jun 8, 2020

View reviewed changes

thomasjpfan reviewed Jun 8, 2020

View reviewed changes

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

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

yagi-3 and others added 5 commits June 9, 2020 13:46

Add change log with |Feature| tag

124236c

Use dtype=object instead of np.object

96f0297

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Use dtype=object instead of np.object

67f4e10

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Fix user link in change log

5ff61f2

Fix highlight in change log

0478d08

rth reviewed Jun 9, 2020

View reviewed changes

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

yagi-3 and others added 3 commits June 9, 2020 22:01

Parametrize unittest

0d6236e

Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>

Change unittest name & remove test_imputation_constant_string_list

95fa552

Rearrange location of test_simple_imputation_string_list

7305859

rth approved these changes Jun 9, 2020

View reviewed changes

rth merged commit 3fb9e75 into scikit-learn:master Jun 9, 2020

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020

Fix simple imputer for string input (scikit-learn#17526)

74142e8

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

Fix simple imputer for string input (scikit-learn#17526)

13796f5

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>

ssaamm mentioned this pull request Nov 17, 2020

[MRG] SimpleImputer: Handle string features where all values are missing #18860

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG]Fix simple imputer for string input #17526

[MRG]Fix simple imputer for string input #17526

yagi-3 commented Jun 7, 2020

alfaro96 Jun 8, 2020

alfaro96 Jun 8, 2020

alfaro96 Jun 8, 2020

alfaro96 left a comment

yagi-3 commented Jun 8, 2020

thomasjpfan left a comment

yagi-3 commented Jun 9, 2020 •

edited

yagi-3 commented Jun 9, 2020

alfaro96 commented Jun 9, 2020

rth left a comment

rth left a comment

reshamas commented Jun 16, 2020

[MRG]Fix simple imputer for string input #17526

[MRG]Fix simple imputer for string input #17526

Conversation

yagi-3 commented Jun 7, 2020

Reference Issues/PRs

What does this implement/fix?

Any other comments?

alfaro96 Jun 8, 2020

Choose a reason for hiding this comment

alfaro96 Jun 8, 2020

Choose a reason for hiding this comment

alfaro96 Jun 8, 2020

Choose a reason for hiding this comment

alfaro96 left a comment

Choose a reason for hiding this comment

yagi-3 commented Jun 8, 2020

thomasjpfan left a comment

Choose a reason for hiding this comment

yagi-3 commented Jun 9, 2020 • edited

yagi-3 commented Jun 9, 2020

alfaro96 commented Jun 9, 2020

rth left a comment

Choose a reason for hiding this comment

rth left a comment

Choose a reason for hiding this comment

reshamas commented Jun 16, 2020

yagi-3 commented Jun 9, 2020 •

edited