Knn imputer implementation by MichaelFu512 · Pull Request #3662 · alteryx/evalml

MichaelFu512 · 2022-08-12T15:54:31Z

Pull Request Description

This is the KNNImputer. I've added it to the hyper-parameter range in `Imputer.py`.

After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of docs/source/release_notes.rst to include this pull request by adding :pr:123.

codecov · 2022-08-12T16:01:50Z

Codecov Report

Merging #3662 (08dbad6) into main (49090e5) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3662     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        337     339      +2     
  Lines      34078   34239    +161     
=======================================
+ Hits       33947   34108    +161     
  Misses       131     131

Impacted Files	Coverage Δ
evalml/tests/automl_tests/test_automl.py	`99.5% <ø> (ø)`
...ts/automl_tests/test_automl_iterative_algorithm.py	`100.0% <ø> (ø)`
evalml/tests/pipeline_tests/test_pipelines.py	`99.8% <ø> (ø)`
...lines/components/transformers/imputers/__init__.py	`100.0% <100.0%> (ø)`
...elines/components/transformers/imputers/imputer.py	`100.0% <100.0%> (ø)`
...es/components/transformers/imputers/knn_imputer.py	`100.0% <100.0%> (ø)`
...components/transformers/imputers/simple_imputer.py	`100.0% <100.0%> (ø)`
evalml/pipelines/components/utils.py	`95.8% <100.0%> (+0.5%)`	⬆️
evalml/tests/component_tests/test_imputer.py	`100.0% <100.0%> (ø)`
evalml/tests/component_tests/test_knn_imputer.py	`100.0% <100.0%> (ø)`
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…yx/evalml into knn_imputer_implementation Git pull

…yx/evalml into knn_imputer_implementation

eccabay

Looking pretty good! Agreed with Jeremy's comments though. I'd also love for you to take a closer look at the tests again - make sure we're testing the new behavior and the weird edge cases, but not throwing too much compute power at unnecessary tests. In general as well, I'd recommend leveraging our pytest fixtures for data creation whenever possible.

eccabay · 2022-08-26T19:29:07Z

 -------------
 **Future Releases**
    * Enhancements
+        *  Added KNNImputer class and created new knn parameter for Imputer? :pr:`3662``


Formatting - need the double tick marks around KNNImputer, plus we don't need the question mark or the extra tick mark at the end.

eccabay · 2022-08-26T19:34:16Z

+    """Imputes missing data using KNN according to a specified number of neighbors.  Natural language columns are ignored.
+
+    Args:
+        number_neighbors (int): Number of nearest neighbors for KNN to search for. Defaults to 0.


Typo: defaults to 0 -> defaults to 3

eccabay · 2022-08-26T19:41:10Z

+    def _drop_natural_language_columns(self, X):
+        natural_language_columns = list(
+            X.ww.select(["NaturalLanguage"], return_schema=True).columns.keys(),
+        )
+        if natural_language_columns:
+            X = X.ww.copy()
+            X = X.ww.drop(columns=natural_language_columns)
+        return X, natural_language_columns
+
+    def _set_boolean_columns_to_categorical(self, X):
+        X_schema = X.ww.schema
+        original_X_schema = X_schema.get_subset_schema(
+            subset_cols=X_schema._filter_cols(exclude=["Boolean"]),
+        )
+        X_boolean_cols = X_schema._filter_cols(include=["Boolean"])
+        new_ltypes_for_boolean_cols = {col: "Categorical" for col in X_boolean_cols}
+        X.ww.init(schema=original_X_schema, logical_types=new_ltypes_for_boolean_cols)
+        return X


These functions are copied from the SimpleImputer but don't rely on being a class method, could you move these into a utils file (both for this class and the SimpleImputer)? The higher level components/utils.py would probably be fine.

eccabay · 2022-08-26T19:45:44Z

+
+        # Drop natural language columns and transform the other columns
+        X_t, natural_language_cols = self._drop_natural_language_columns(X)
+        if X_t.shape[-1] == 0:


Nit: Just a few lines higher you check X.shape[1] while here you check X_t.shape[-1]. Can you flip this to be consistent?

eccabay · 2022-08-26T19:52:41Z

+        "numeric_impute_strategy": ["mean", "median", "most_frequent", "knn"],
+        "boolean_impute_strategy": ["most_frequent", "knn"],


We're going to need more tests here than just checking that knn is in the hyperparameters. At a minimum, I'd test when a dtype strategy is set to knn that the class attribute is a KNN imputer with the expected parameters.

eccabay · 2022-08-26T19:57:17Z

+from evalml.pipelines.components.transformers.imputers import KNNImputer
+
+
+def test_knn_imputer_1_neighbor():


We don't need to test this. Fundamentally, we can rely on sklearn to have implemented n_neighbors correctly. A better thing to test here would be to check that our output matches the sklearn object's output, and we can parametrize over the number of neighbors if you're worried about that as well.
Pseudocode:

@pytest.mark.parametrize("n_neighbors", [1, 2, 5]) def test_knn_output(n_neighbors): X = ... sk_knn = SKKNN(n_neighbors) knn = KNN(n_neighbors) assert sk_knn.fit_transform(X) == knn.fit_transform(X)

eccabay · 2022-08-26T20:00:47Z

+    assert_frame_equal(X_multi_expected_arr, X_multi_t)
+
+
+def test_knn_imputer_all_bool():


This test is almost functionally identical to test_knn_imputer_all_bool_return_original, we don't need both

…yx/evalml into knn_imputer_implementation

eccabay

Looking really good! Just a few small things to fix before this goes in

eccabay · 2022-09-01T14:58:51Z

+        original_X_schema = X_schema.get_subset_schema(
+            subset_cols=X_schema._filter_cols(exclude=["BooleanNullable"]),
+        )
+        X_bool_nullable_cols = X_schema._filter_cols(include=["Categorical"])
+        new_ltypes_for_bool_nullable_cols = {
+            col: "Boolean" for col in X_bool_nullable_cols
+        }


I'm very confused by this and the lower code messing with boolean data types. What exactly what the goal here? It looks like you may be able to replace all this with a call to downcast_nullable_types.

I rewrote the code above since honestly it confused even me reading it now. downcast_nullable_types turns type booleans into doubles which I would prefer not to have happen.

Hm, downcast_nullable_types appears to turn BooleanNullable into boolean, if you were getting doubles that seems like a problem we should address. It's not relevant to this issue though, and the cleaned up code is a lot easier to read so I'm fine with it!

eccabay · 2022-09-01T15:05:07Z

    return component_class


+def drop_natural_language_columns(X):


Thanks for doing this! It'd be great if you could add test cases for this function as well as set_boolean_columns_to_categorical.

jeremyliweishih

Good work! Should be good to go after some more testing changes as well as Becca's comments.

jeremyliweishih · 2022-09-01T17:14:57Z

-        )
+        if boolean_impute_strategy == "knn":
+            self._boolean_imputer = KNNImputer(
+                number_neighbors=1,


I think thats a good idea - we should be able to up the number of neighbors. Can you file an issue for this?

jeremyliweishih · 2022-09-01T17:25:33Z

+    imputer = KNNImputer()
+    imputer.fit(X, y)
+    X_t = imputer.transform(X)
+    assert_frame_equal(X_expected_arr, X_t)


let's also check the woodwork logical types are the same here.

jeremyliweishih · 2022-09-01T17:28:05Z

+            "some_nan": pd.Series([True, np.nan, False, np.nan, True], dtype="boolean"),
+        },
+    )
+    X.ww.init(logical_types={"some_nan": "BooleanNullable"})


should we only init here based off if data_type == ww? we should either init or run make_data_type later.

jeremyliweishih · 2022-09-01T17:28:56Z

+    imputer = KNNImputer(number_neighbors=1)
+    X_t = imputer.fit_transform(X, y)
+    X_ww_expected = "<Series: some_nan (Physical Type = bool) (Logical Type = Boolean) (Semantic Tags = set())>"
+    assert str(X_t.ww["some_nan"].ww) == X_ww_expected


you can just assert that the logical type is Boolean by indexing into X_t.ww

jeremyliweishih · 2022-09-01T17:29:15Z

+            "bool no nan": pd.Series([False, False, False, False, True], dtype=bool),
+        },
+    )
+    X_multi.ww.init(logical_types={"bool with nan": "BooleanNullable"})


same question about init and make_data_type here.

jeremyliweishih · 2022-09-01T17:29:33Z

+    imputer = KNNImputer(number_neighbors=1)
+    imputer.fit(X_multi, y)
+    X_multi_t = imputer.transform(X_multi)
+    assert_frame_equal(X_multi_expected_arr, X_multi_t)


let's assert that the logical types make sense here too.

jeremyliweishih · 2022-09-01T17:31:19Z

+    assert_frame_equal(X_multi_expected_arr, X_multi_t)
+
+
+def test_knn_imputer_revert_categorical_to_boolean():


is this test to check if BooleanNullables will be converted to Boolean? If so let's parameterize this method to use pandas and ww as well and then check the logical types in the WW case.

Line 92 was to test if I could revert categorical to boolean. However KNNImputer doesn't have the same issues that SimpleImputer has if you pass in just bools (no nulls), therefore I can get rid of line 92's test (since I don't have to actually change bool columns that are all bools into categorical).

jeremyliweishih · 2022-09-01T17:31:46Z

+
+    result = imputer.transform(X_df, y)
+
+    # raise Exception


is this comment necessary?

eccabay

Awesome, thanks for powering through this! Let's get this in 😎

MichaelFu512 added 4 commits August 10, 2022 09:43

initial changes

c96c616

added knnimputer to imputer.py

4b6e866

added knnimputer to imputer.py

fd1aa2a

Merge branch 'main' into knn_imputer_implementation

6376adc

MichaelFu512 added 25 commits August 12, 2022 09:20

lint issues

800cb69

Merge branch 'knn_imputer_implementation' of https://github.com/alter…

a429ec4

…yx/evalml into knn_imputer_implementation Git pull

pre-commit runn

a62bb29

Merge branch 'main' into knn_imputer_implementation

0b181e6

fixed test_imputer.py

d6c67d1

Merge branch 'knn_imputer_implementation' of https://github.com/alter…

7074c15

…yx/evalml into knn_imputer_implementation

update release notes

538f61f

fixed some tests

35203f2

fixed more tests

0038130

Update imputer.py

499caf6

fixed some things

3c0252a

Merge branch 'knn_imputer_implementation' of https://github.com/alter…

999eee8

…yx/evalml into knn_imputer_implementation

made knn the default strategy

e50799a

made knn not go first and added tests

86a0684

Merge branch 'main' into knn_imputer_implementation

7a68e59

Merge branch 'main' into knn_imputer_implementation

d6eaeca

more tests, made knn convert bools in bools

94b5dda

lint

d8916c2

removed random comment

fee2ee1

Merge branch 'main' into knn_imputer_implementation

9567899

Merge branch 'knn_imputer_implementation' of https://github.com/alter…

f34bc11

…yx/evalml into knn_imputer_implementation

release notes update

2ddb34f

updated tests

37fee1e

testing changes

76418b9

Merge branch 'main' into knn_imputer_implementation

e1d2d09

Updated knn_imputer_test

a0bd6cf

eccabay suggested changes Aug 26, 2022

View reviewed changes

MichaelFu512 added 15 commits August 26, 2022 13:39

release notes change

ab96204

Update test_knn_imputer.py

85104f3

knn_imputer changes

82dcaf8

knn_imputer_test changes

511b56d

simple imputer changes

d767c6c

test simple imputer + test imputer

de771d9

added 2 utils that came from simple imputer

0ce26c2

Merge branch 'main' into knn_imputer_implementation

a7870e2

Merge branch 'knn_imputer_implementation' of https://github.com/alter…

566d04e

…yx/evalml into knn_imputer_implementation

uncommented set boolean columns to cat

8774c8a

added docstring for utils.py

92877a7

lint fix

9613406

removed random print

295344d

more changes

36f1227

removed unncessary import

4e06a47

MichaelFu512 requested review from eccabay and jeremyliweishih August 31, 2022 15:50

eccabay reviewed Sep 1, 2022

View reviewed changes

Merge branch 'main' into knn_imputer_implementation

21ec2ed

jeremyliweishih approved these changes Sep 1, 2022

View reviewed changes

MichaelFu512 added 4 commits September 1, 2022 12:02

Merge branch 'main' into knn_imputer_implementation

bab1d39

Merge branch 'main' into knn_imputer_implementation

e98157a

knn imputer changes and test changes

5e5b391

test utils changes

08dbad6

MichaelFu512 requested a review from eccabay September 2, 2022 16:21

eccabay approved these changes Sep 2, 2022

View reviewed changes

MichaelFu512 merged commit de675e9 into main Sep 2, 2022

MichaelFu512 deleted the knn_imputer_implementation branch September 2, 2022 17:25

		"numeric_impute_strategy": ["mean", "median", "most_frequent", "knn"],
		"boolean_impute_strategy": ["most_frequent", "knn"],

		from evalml.pipelines.components.transformers.imputers import KNNImputer


		def test_knn_imputer_1_neighbor():

		assert_frame_equal(X_multi_expected_arr, X_multi_t)


		def test_knn_imputer_all_bool():

		assert_frame_equal(X_multi_expected_arr, X_multi_t)


		def test_knn_imputer_revert_categorical_to_boolean():

Conversation

MichaelFu512 commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Description

This is the KNNImputer. I've added it to the hyper-parameter range in Imputer.py.

Uh oh!

codecov Bot commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eccabay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eccabay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelFu512 Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremyliweishih left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eccabay left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MichaelFu512 commented Aug 12, 2022 •

edited

Loading

This is the KNNImputer. I've added it to the hyper-parameter range in `Imputer.py`.

codecov Bot commented Aug 12, 2022 •

edited

Loading

MichaelFu512 Sep 1, 2022 •

edited

Loading