Fix TargetEncoder compatibility with scikit-learn ColumnTransformer #461
Conversation
|
This fix looks great, code is cleaner than before, test is also nice! Great job and sorry again for having you wait so long. |
…emonstrate the failure detailed in issue 460. scikit-learn-contrib#460
… that clone the object and validate that the parameters are unchanged. This moves the validation of hierarchy and the generation of an inverted hiearchy to the helper function _generate_inverted_hierarchy. This function is called in __init__ only to keep compatibility with existing tests by throwing an exception from the constructure rather than waiting for the call to fit. We create a variable inverted_hierarchy rather than overwriting self.hierarchy in order for the clone method to pass validation.
dennisobrien
force-pushed
the
issue-460-fix_targetencoder
branch
from
ac4bc92 to
106af76
Compare
Removed ToDo comment based on feedback. Added type hint to test_hierarchy_with_scikit_learn_column_transformer.
Thanks for the feedback. And no problem about the timeline. I appreciate your contributions in creating and maintaining this project. I made the changes you mentioned and followed up in each of the comments. Let me know if there is anything else you'd like me to change in this. |
|
LGTM & pipeline passing. I'm merging |
PaulWestenthanner
merged commit 370e4b8
into
scikit-learn-contrib:master
2 participants
Fixes #460
Proposed Changes
TargetEncoderpassed a non-nullhierarchyobject would throw an exception when used in ascikit-learnColumnTransformer.TargetEncoder__init__method to thefitmethod in order to pass validation tests in thescikit-learnclonemethod.test_hierarchy_with_scikit_learn_column_transformer.