Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug where PerColumnImputer cannot handle dropped columns #855

Merged
merged 15 commits into from Jun 16, 2020

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Jun 15, 2020

Closes #514 by addressing PerColumnImputer

@angela97lin angela97lin self-assigned this Jun 15, 2020
@codecov
Copy link

codecov bot commented Jun 15, 2020

Codecov Report

Merging #855 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #855   +/-   ##
=======================================
  Coverage   99.69%   99.69%           
=======================================
  Files         195      195           
  Lines        7930     7962   +32     
=======================================
+ Hits         7906     7938   +32     
  Misses         24       24           
Impacted Files Coverage Δ
...onents/transformers/imputers/per_column_imputer.py 100.00% <100.00%> (ø)
...l/tests/component_tests/test_per_column_imputer.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4167aaa...25ec13e. Read the comment docs.

@@ -50,7 +51,7 @@ def fit(self, X, y=None):
strategy_dict = self.impute_strategies.get(column, dict())
strategy = strategy_dict.get('impute_strategy', self.default_impute_strategy)
fill_value = strategy_dict.get('fill_value', None)
self.imputers[column] = SkImputer(strategy=strategy, fill_value=fill_value)
self.imputers[column] = SimpleImputer(impute_strategy=strategy, fill_value=fill_value)
Copy link
Contributor Author

@angela97lin angela97lin Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than use SkImputer, I'm using our SimpleImputer which already handles the nan's from the previous PR :)

Copy link
Collaborator

@dsherry dsherry Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯 oh, great idea!! Modularity always wins 👍😁

@@ -67,8 +68,14 @@ def transform(self, X, y=None):
pd.DataFrame: Transformed X
"""
X_t = X.copy()
cols_to_drop = []
Copy link
Contributor Author

@angela97lin angela97lin Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't set column to empty dataframe, so updating to check first.

@angela97lin angela97lin requested a review from dsherry Jun 15, 2020
Copy link
Collaborator

@dsherry dsherry left a comment

Good stuff! 🚢

@@ -50,7 +51,7 @@ def fit(self, X, y=None):
strategy_dict = self.impute_strategies.get(column, dict())
strategy = strategy_dict.get('impute_strategy', self.default_impute_strategy)
fill_value = strategy_dict.get('fill_value', None)
self.imputers[column] = SkImputer(strategy=strategy, fill_value=fill_value)
self.imputers[column] = SimpleImputer(impute_strategy=strategy, fill_value=fill_value)
Copy link
Collaborator

@dsherry dsherry Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯 oh, great idea!! Modularity always wins 👍😁

@angela97lin angela97lin merged commit b2ce69f into master Jun 16, 2020
2 checks passed
@angela97lin angela97lin deleted the 514_percol branch Jun 16, 2020
@angela97lin angela97lin mentioned this pull request Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SimpleImputer cannot handle dropped columns
2 participants