-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
Don't error when DropFeatures get's empty list #567
Comments
Hi @sTomerG Thanks for the suggestion. This is a use of Going back to my thinking when I designed this class: it was intended for users to remove features based on some domain knowledge. Thus, calling this class and removing no feature (empty list) made no sense. Hence, the error check. Of course I did not see the potential use in combination with GridSearch. To try and understand how common this would be, could you explain a bit more of the scenario in which you use it together with a gridsearch? I guess you try different feature combinations, plus the empty list. Why would you enter different feature combinations manually? Re the code change: I'd say, if you already created a solution (removing the check + test), then go ahead and make a PR. |
Thanks for your quick reply @solegalli :). Now that I've thought about it some more; this actually applies to more classes in feature-engine which have a parameter that specifies on which variables the transformation should be applied. With categorical encoding, you might want to gridsearch whether the model learns more of the feature through ordinal encoding or through mean encoding. This would be possible if the parameter
This would result in To me, this feels like it makes sense because variables are "The list of categorical variables that will be encoded.", hence: empty list, no variables encoded. |
I see the point for automation. But I also think that removing the "safety net" of alerting the user when no variables will be transformed by the encoder is too dangerous, i.e., the user may think that their variables are being transformed, but they are not. There could be options with Optuna to sample the param space with no variables. Optuna allows to define parameters with the define-by-run method. If you have a real code example, we could try to figure out how to do it, and then maybe create some example in the documentation for others to look at. |
Hi! I've found a solution; I've created a meta-estimator which can turn other estimators/transformers in a Pipeline on or off :)
|
I guess this is resolved now, so I am closing this issue. Thanks for sharing your implementation and solution @sTomerG |
Yes closing is fine! @solegalli If you're interested, in this article I've explained strategies on how to use feature-engine's transformers in a grid search :) |
Thank you @sTomerG ! I shared on social :p |
Is your feature request related to a problem? Please describe.
To be able to more easily gridsearch over which features to drop using
DropFeatures
it would be helpful ifDropFeatures
wouldn't generate aValueError
whenlen(features_to_drop) == 0
, so that you can also choose to not drop any features in the gridsearch by inserting an empty list as value.Describe the solution you'd like
remove
or len(features_to_drop) == 0
from line 58 in drop_features.pyDescribe alternatives you've considered
Forking the library and removing the line of code myself (which I already did), but I might think this improvement could help other people as well.
The text was updated successfully, but these errors were encountered: