Fix DropFeatures fail when features_to_drop is string, fixes issue 406 - #407
Conversation
| "you wish to drop from the dataframe." | ||
| ) | ||
|
|
||
| if isinstance(features_to_drop, str): |
There was a problem hiding this comment.
To follow sklearn conventions, we must not re-assign user' entered values in the init parameters.
Here, we need to let feature_to_drop be a string, and make it a list when we create features_to_drop_ in the fit method.
I am surprised that the check_estimator from sklearn did not pick this up. I was expecting the test to fail :_(
There was a problem hiding this comment.
I am wondering... if in this line we take 0 out of the list, so that DropFeatures(features_to_drop="0"), would then the error be raised with the current logic?
Would you check?
| raise ValueError( | ||
| "features_to_drop should be a list with the name of the variables" | ||
| "features_to_drop should be a list with the name of the variables " | ||
| "you wish to drop from the dataframe." |
There was a problem hiding this comment.
since we are working on this class... can we add f"Got {features_to_drop} instead." to the error message?
Thank you!
solegalli
left a comment
solegalli
left a comment
There was a problem hiding this comment.
Thank you @noahjgreen295 !!
A tiny change requested, to follow conventions...
|
OK made both changes!
|
|
Thank you!!! |
2 participants
Fix to issue #406 , where
DropFeaturesraises exception when instantiating withfeatures_to_dropas string, even though docs and type hinting says string is supported.