Skip to content
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

Multi class shap rfecv #174

Merged
merged 15 commits into from
Dec 5, 2023
Merged

Conversation

lproth
Copy link
Contributor

@lproth lproth commented Nov 24, 2021

Re #169 I have added support for multi-class classification and ensured that regression will still work. I haven't written any unit tests because I'm not familiar with how pytest works, but all that should be needed is to re-run the existing tests with a multi-class target instead of binary.

multi-class classifiers and regressors are now compatible, so we remove
references to binary classification
Copy link
Collaborator

@Matgrb Matgrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this, and extending the functionality for other users.
I left some comments to implement and questions, but overall looks good.

probatus/feature_elimination/feature_elimination.py Outdated Show resolved Hide resolved
probatus/utils/arrayfuncs.py Show resolved Hide resolved
probatus/utils/shap_helpers.py Show resolved Hide resolved
probatus/feature_elimination/feature_elimination.py Outdated Show resolved Hide resolved
probatus/feature_elimination/feature_elimination.py Outdated Show resolved Hide resolved
@Matgrb
Copy link
Collaborator

Matgrb commented Dec 8, 2021

Also please have a look what has changed in, just to make sure your PR is aligned #175

Copy link
Collaborator

@Matgrb Matgrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the work and sorry for the delay.

If you fix the conflicts and make the test sucessfully run we can directly merge ;) Keep in mind that there are 3 new functions added for EarlyStoppingRFECV that pass early stopping parameters differently based on model type: LGBM/XGB/CatBoost

@lproth
Copy link
Contributor Author

lproth commented Jan 6, 2023

Hi @Matgrb, can we merge this?

@ReinierKoops
Copy link
Contributor

ReinierKoops commented Jun 4, 2023

Hi @lproth , sorry for the delayed response and thank you for your contribution, could you update this branch to be compatible with the latest changes on the main?

@ReinierKoops ReinierKoops self-requested a review June 4, 2023 06:10
@ReinierKoops ReinierKoops self-assigned this Jul 6, 2023
@ReinierKoops
Copy link
Contributor

This can only be merged when some unit tests are added.

@ReinierKoops ReinierKoops removed their request for review July 6, 2023 17:03
ReinierKoops
ReinierKoops previously approved these changes Dec 5, 2023
Copy link
Contributor

@ReinierKoops ReinierKoops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update has been approved in previous commits and now just updates to be compatible with main

@ReinierKoops ReinierKoops dismissed their stale review December 5, 2023 19:07

no actual unit test added

Copy link
Contributor

@ReinierKoops ReinierKoops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed some bugs & added a unit test. removed spurious code.

@ReinierKoops ReinierKoops merged commit a5088ef into ing-bank:main Dec 5, 2023
12 checks passed
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.

None yet

4 participants