Add min_impute and nan warning #423
Conversation
There was a problem hiding this comment.
@grromrell you've got some really nice work here and I left a few suggestions to tighten it up. Happy to discuss more. Welcome aboard! (feel free to hit me up on slack - find an invite on healthcare.ai)
healthcareai/common/transformers.py
Outdated
| @@ -20,11 +21,12 @@ class DataFrameImputer(TransformerMixin): | |||
| Columns of other types (assumed continuous) are imputed with mean of column. | |||
| """ | |||
|
|
|||
| def __init__(self, impute=True, verbose=True): | |||
| def __init__(self, impute=True, verbose=True, max_impute=.5): | |||
There was a problem hiding this comment.
- Great idea. Would you mind keeping the
verbosearg last? - At this point I think that this method needs a docstring. We prefer google format, and if you're using an editor like pycharm it's trivial to get that formatting right. No need to go overboard, just note what the arguments mean.
healthcareai/common/transformers.py
Outdated
| for c in X: | ||
| pct_impute = X[c].isnull().sum() / len(X) | ||
| if pct_impute > self.max_impute: | ||
| warnings.warn("{0:.2f}% of data for column '{1}' is missing. Imputed " |
There was a problem hiding this comment.
This might read a little better if you state the column name first, then the %missing. I totally love this, and I'm going to steal your idea of using warnings for a few other thigns!
healthcareai/common/transformers.py
Outdated
| RuntimeWarning) | ||
|
|
||
| #Alternative fill, only fill maximum number of values based on max_impute | ||
| #result = X.fillna(self.fill, limit=len(X)//(1/self.max_impute)) |
There was a problem hiding this comment.
I'm not sure how this will affect downstream algorithms. Do you have any sense for this? If not, I'd say create another issue and remove this commented code from this PR.
| @@ -54,7 +56,17 @@ def transform(self, X, y=None): | |||
| # Return if not imputing | |||
| if self.impute is False: | |||
| return X | |||
|
|
|||
|
|
|||
| #Warn users if %nan is too high | |||
There was a problem hiding this comment.
I'd be thrilled if this were factored out to a separate function with tests (and I'm happy to guide you through that if you'd like).
|
I added some docs, changed the way the warning prints and removed the alternative fill method. I did not separate the warn_nan into its own function as it seems like this is the only place it would get used and adding a unit test didn't seem worth it for a simple warning. |
Fixing #421
Just adds some runtime warnings to the DataFrameImputer. I also added a potential change to the X.fillna that could be used to limit the amount of imputing done by using the limit keyword. I thought it was worth considering as an option. Also not married to the value for max_impute, thoughts there are helpful.