-
Notifications
You must be signed in to change notification settings - Fork 6
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
Deal with grouped/ungrouped DataFrames in Steps #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add typehints and docstrings for the added methods? (And maybe ideally for every touched method, that way we'll get there little by little for the 'legacy' code as well.)
@HendrikSchmidt I've added docstrings and typehints, thanks for pointing this out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to refactor new_data = self._check_ingredients(data, remove_group=True)
into a similar transform
and do_transform
pattern like with fit
?
Note that my test for impute fill introduced a recipe with NaN but such a recipe already seems to exist in another branch in conftest.py. I need to resolve this. |
Addresses the issues described in #13
The approach taken here is to use a function
_check_ingredients
that a) checks if a step accepts grouped dataframes and throws an error if it doesn't and b) removes the group if asked to via theremove_group
parameter. See individual Steps for details.