extend drop psi functionality to categorical variables

feature-engine / feature_engine

Feature engineering package with sklearn like functionality

https://feature-engine.trainindata.com/

BSD 3-Clause "New" or "Revised" License

1.8k stars 303 forks source link

extend drop psi functionality to categorical variables #664

Closed solegalli closed 1 year ago

solegalli commented 1 year ago

closes #655 closes #658 closes #657

solegalli commented 1 year ago

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:

I mostly tidied the docstrings
fixed doc error
did minor cosmetics in the class code
added some tests for p-value

Would you be able to have a quick look and if OK we merge?

Thank you!

dlaprins commented 1 year ago

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.

solegalli commented 1 year ago

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.

dlaprins commented 1 year ago

A bit unwieldy for future use, but I suppose backwards compatibility takes precedence. I'm in favor.