-
-
Notifications
You must be signed in to change notification settings - Fork 324
extend drop psi functionality to categorical variables #664
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
Conversation
Hey @dlaprins and @ClaudioSalvatoreArcidiacono I've got the impression that rebasing didn't work in #657 because I can see the files that we merged in #660 in there. I tried to rebase again, but got a lot of errors, so decided to start afresh. The changes here in summary:
Would you be able to have a quick look and if OK we merge? Thank you! |
Hi @solegalli, I'm not sure what went wrong when I rebased, but it must have done something since all unit tests except for the docstring one which failed before now passed. In any case, I reviewed this PR, no comments from my side on the code changes. |
Thank you @dlaprins I am considering now on adding an option to the variables parameter in the init, not to break backwards compatibility. In the live version, if variables=None, the transformer will operate only on numerical variables. If we deploy this version as is, when variables=None the transformer will operate on all variables, and thus break backward compatibility. So I am thinking of leaving variable=None with the current behaviour and adding the option variables="all" to operate on all variables. |
A bit unwieldy for future use, but I suppose backwards compatibility takes precedence. I'm in favor. |
closes #655
closes #658
closes #657