Skip to content

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

Merged
merged 15 commits into from
May 4, 2023

Conversation

solegalli
Copy link
Collaborator

closes #655
closes #658
closes #657

@solegalli
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

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
Copy link

dlaprins commented May 3, 2023

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

@solegalli solegalli merged commit dade0f0 into main May 4, 2023
@solegalli solegalli deleted the extend_psi_to_categorical branch May 4, 2023 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PSI feature selection for categorical features
2 participants